eui

eui copied to clipboard

Summary

The gap property in flexbox has been supported by all major browsers (and all supported Elastic browsers) as of April 2021. We should move away from it instead of making use of negative margins, both for simplicity and because negative margins can occasionally cause unexpected DOM or cross-browser side effects (For example: the 2nd Safari issue in this comment that occurs when using negative margins and horizontal scrolling, which is solved by switching to gap).

Links:

• https://caniuse.com/flexbox-gap
• https://developer.mozilla.org/en-US/docs/Web/CSS/gap#browser_compatibility
• https://coryrylan.com/blog/css-gap-space-with-flexbox

QA

• [ ] Check https://eui.elastic.co/pr_5575/#/layout/flex against https://elastic.github.io/eui/#/layout/flex and confirm no visual regressions

Checklist

• [ ] Check against all themes for compatibility in both light and dark modes
• [ ] Checked in mobile
• [ ] Checked in Chrome, Safari, Edge, and Firefox

~- [ ] Props have proper autodocs and playground toggles~ ~- [ ] Added documentation~ ~- [ ] Checked Code Sandbox works for any docs examples~ ~- [ ] Added or updated jest and cypress tests~

• [ ] Checked for breaking changes and labeled appropriately

~- [ ] Checked for accessibility including keyboard-only and screenreader modes~

• [ ] A changelog entry exists and is marked appropriately

@snide helped provide some context that this will likely impact a decent amount of downstream consumers. Cross-posting here for visibility:

where things will break is that i know for certain there are likely hundreds of overwrites throughout the company that "fix" margins on them. so the base will be fine, but those overwrites will still apply, and break separately

Where that concerns this PR:

Likely we will want to consider how we migrate more than the change in the code. Whether that's deprecation with a prop setting, an actual separate component...etc

Would love to get feedback on the upgrade approach that people think is most realistic:

1. Switching completely and checking Kibana, Cloud, Dream Machine, Eden, etc. for possible breaking/overrides to update when we upgrade
2. Switch completely but add a useNegativeMargins prop for consumers that see breakages and need an escape hatch to revert to negative margins
3. Do not switch the default gutter CSS, and add a new useGap prop that toggles gap CSS (how would we indicate the non-gap CSS will be deprecated however?)
4. Do not switch the default gutter CSS, add a new gapSize prop to apply new CSS, and mark the old gutterSize as deprecated so that consumers can update on their own schedule (how would handling a default EuiFlexGroup without a specified gutterSize work however?)

Preview documentation changes for this PR: https://eui.elastic.co/pr_5575/

I love this!

Options 3 and 4 are the safest answer. I think I'd prefer 3, but I'll go through the process of thinking about 4.

3. Do not switch the default gutter CSS, and add a new useGap prop that toggles gap CSS (how would we indicate the non-gap CSS will be deprecated however?)

I think hooks with use. Maybe isGap? For deprecation, I'd handle it through docs and traditional messaging. I like this one because it's opt in and the prop can disappear after deprecation.

4. Do not switch the default gutter CSS, add a new gapSize prop to apply new CSS, and mark the old gutterSize as deprecated so that consumers can update on their own schedule (how would handling a default EuiFlexGroup without a specified gutterSize work however?

In cases where there was no gutterSize, but gapSize was set, i'd use gap. In cases where neither gutterSize or gapSize is not set, I would continue using gutterSize's default. This gives folks an active, non-breaking way to apply the preffered method with zero-breaks on merge. Our docs should be changed to use gapSize in all examples, and along with some communication, a natural, no-pressure update could happen.

If we went invasive, I would suggest planning it as the start of the next minor, and going door to door to individual developers to explain this change.

I'll provide my contrary 2 cents to Dave's, which is that I prefer option 4 (a new gapSize prop). Adding a temporary prop isGap force consumers to initiate two upgrade paths (add isGap, remove after deprecation) versus just one. Adding a new prop gapSize allows a very easy find/replace upgrade from gutterSize -> gapSize without much needed after thought.

I'd then propose the following stages:

1. Now

• Add gapSize that follows the same sizing that gutterSize does
• Change just the default/unchanged gutterSize to use gapSize instead.

gapSize: 'l',
gutterSize: 'none',

• Set gutterSize to deprecated; Update docs to reflect this

2. Update EUI internal components

• Convert internal components that use gutterSize to use gapSize and fix any bugs

3. CSS-in-JS conversion

• Remove gutterSize prop, prompting consumers to switch to gapSize with easier customization than just t-shirt sizes

// example:
gapSize = 'l' || 64

This gives consumers the customization they need in order to remove their margin overrides and more ideal.

I know I mostly forgot about this PR while doing other things, but it looks like now might be a good time to re-raise it during the CSS-in-JS conversion. @cchaos just wanted to check back in on this - it looks like based on the checklist item in https://github.com/elastic/eui/issues/5685#issuecomment-1092255627 we might be converting EuiFlex to gap as part of the Emotion conversion? Or is that for components other than EuiFlex?

@constancecchen Yeah, that checklist item is more geared towards styles that aren't controlled by the consumer. If we do deprecate gutterSize we need a long deprecation period. I still like my plan outlined above, just that the new gap would be added now with that added flexibility but don't remove gutterSize yet.

👋 Hey there. This PR hasn't had any activity for 90 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

Closing this PR in favor of https://github.com/elastic/eui/pull/6270. We've put out a Kibana/Cloud-wide survey and all responses so far have been largely in favor of switching away from margins to gap without any blockers or request for backward compatibility, so I'm making the switch as part of the components EUI conversion.