prevent modalBody render if no children provided

[thompsongl]

Summary

Resolves #1382.

Prevents empty EuiModalBody from rendering in EuiConfimModal

Checklist

- [ ] This was checked in mobile
- [ ] This was checked in IE11
- [ ] This was checked in dark mode
- [ ] Any props added have proper autodocs

• Documentation examples were added
• A changelog entry exists and is marked appropriately
• This was checked for breaking changes and labeled appropriately
• Jest tests were updated or added to match the most common scenarios

- [ ] This was checked against keyboard-only and screenreader scenarios
- [ ] This required updates to Framer X components

[chandlerprall]

Prefer testing explicitly for children.length > 0. Less mental hoops to jump through when reading through the code.

[cjcenizal]

I like the thinking behind this change, but what if the user provides a string of whitespace like ' '? Then we could make this children.trim().length > 0. Though it's a bit of a slippery slope -- at which point do we stop sanitizing input and just trust the consumer to provide us with what they want?

For questions like this, my reasoning tends to take a draconian approach: "This component can't know what the consumer intends, but it can provide predictable behavior. I can't think of a reason for why the consumer would want to provide an empty string, but there is a limit to my imagination and no limit to the number of consumers' use cases for this component now and in the future. Therefore, providing predictable behavior and making no assumptions about the consumer's intention, even if it seems silly, is the optimal choice."

Not saying we shouldn't do this, just thought I'd share how I would approach things. 😄

[chandlerprall]

I think this change reflects the spirit of the component. An empty string is empty contents, but a non-empty string (including whitespace) is contentful. This provides the predictability of the component ('' is empty) while allowing flexibility of passing ' '.

[cjcenizal]

🚀 This looks great! Thanks for making this change. What do you think of adding an example so it's easy for people to see how it looks?

[cchaos]

If you do add a doc example, I wouldn't label it as empty since that is a misnomer for what the example is actually achieving. I'd label it something like title only.

[thompsongl]

New thought: We can do this without introducing breaking changes (:tada:)

A CSS-only solution exists that would:

1. Leave all relevant data-test-subj attributes in place for testing framework selection
2. "Hide" the modal body via conditional className addition

I'll get changes pushed soon

[cjcenizal]

We can do this without introducing breaking changes

I was under the impression that the change as-is (removing the DOM element) was not a breaking change -- what am I missing?

[thompsongl]

@cjcenizal This is something of a discussion with @chandlerprall, but testing hooks are considered part of EUI's public API. Conditionally rendering the modal body removes its child data-test-subj, which is likely to break integration tests.

[chandlerprall]

Perhaps curiously, I'm okay with that change not being breaking - if a test cares about confirmModalBodyText and it isn't passing content in, that test isn't a very good one. Copying from my reply above: that reflects the spirit of the component

[cchaos]

Please, completely ignore my comment. Go with the original PR......

[cchaos]

Approved!!

[cjcenizal]

@thompsongl @chandlerprall I was thinking along the same lines as Chandler -- if anything, a test would expect the body to be empty and therefore would want to assert that the body isn't rendered. I think you could consider this a bug fix. If consumers are dependent upon a bug, which we then fix, would we consider that breaking?

[thompsongl]

Alright y'all. Thank you for the discussion and your perspectives on how tests impact breaking changes.

I've remained with the original changeset and added a docs example.

[chandlerprall]

LGTM!