Moving EuiBreadcrumb styles to Emotion. #5934

1Copenut · 2022-05-27T00:38:39Z

Summary

Moved the EuiBreadcrumb to Emotion CSS-in-JS. @cchaos mentioned to split the PR for breadcrumbs and page header breadcrumbs. I'll review this before moving the PR out of draft, but it appears the core breadcrumb CSS was transferring to the page header. I couldn't see any visual differences between my local server and the published version.

Checklist

* Adding basic Emotion styles for links and content. * Adding the truncated link and content styles to Emotion. * Added all rules to Emotion. Testing for visual regression. * Fixed the focus halo for overflow:hidden declarations. * Removed breadcrumb overrides from Amsterdam theme. * Added last styles for individual truncated content. * Final Emotion style updates and a11y QA testing complete. * Updating snapshot tests for breadcrumb Emotion styles.

kibanamachine · 2022-05-27T01:05:16Z

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

cchaos · 2022-05-27T02:53:53Z

Hey, sorry, not the PageHeader breadcrumbs, the EuiHeaderBreadcrumbs as seen on this page: https://eui.elastic.co/pr_5934/#/layout/header

kibanamachine · 2022-07-13T20:52:43Z

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

* Removing first group of SCSS files ahead of Amsterdam overrides. * Moving Amsterdam overrides to Emotion. * Added majority of the header breadcrumb styles. * Header styles are now contained, not casting into regular breadcrumbs. * Last color and bgColor styles added. * Removed Amsterdam SCSS overrides file. * Updating Jest snapshots for Emotion. * Adding CHANGELOG, final mobile styles.

1Copenut

Left one comment about the reasoning for having a new prop. LMK what you think.

1Copenut · 2022-07-14T21:54:29Z

src/components/breadcrumbs/breadcrumbs.styles.ts

+        }
+      }
+    `,
+    isHeaderBreadcrumb: css`


The EuiHeaderBreadcrumbs didn't have an easy way to cast styles into child elements, so I added a headerBreadcrumb boolean prop so we could target header breadcrumbs.

I don't think this will be a breaking change because I passed the headerBreadcrumb prop into the EUIHeaderBreadcrumbs component, so it'll get picked up automatically and cast the new styles. If the reviewers feel this will be a breaking change, I'm happy to refactor.

Not a breaking change! I ended up following this pattern and adding another isNested prop (used for breadcrumbs within the collapsed popover - e50a5f9). It's much cleaner to handle this logic via JS rather than nested CSS.

I'm not 100% on this but I'm not a super huge fan of the headerBreadcrumb/isHeaderBreadcrumb var name. It feels super duper specific when there's arguably a use case for the visual style outside of EuiHeader. I'm wondering if a prop name like isBadge or isChevron would be more descriptive. @1Copenut @miukimiu any thoughts here?

I'm on board for a more descriptive prop name. I used isHeaderBreadcrumb purely because it fit with the presentation naming / spaces in EUI. I agree that it could be improved on if these styles can exist outside EuiHeader. Great call!

It's now a boolean flag, so super easy to see the visual effect on the playground!

It's just kind of a question of intention in visual language, I suppose. If we're OK with the chevron effect being used in places other than EuiHeader, then it makes sense to name it more generically. If it's only supposed to be used by EuiHeader, then it makes sense to be more specific and document it as such. I don't know for sure though, hence why I'm tagging in our designers.

That's an excellent way to visualize and document the conditional styling.

The EuiHeaderBreadcrumb was added by @cchaos and because she didn't add the styles directly in EuiBreadcrumb I'm assuming she intentionally only wanted those styles to appear for EuiHeader.

But that's my assumption... Caroline is back on Monday so I guess we can wait and she let us know. 😄

Right, agreed. I want to note that we could move the styles to src/components/header/header_breadcrumbs instead of remaining in src/components/breadcrumb, but there are a few (minor) disadvantages to doing so vs keeping all the styles in breadcrumb.styles.ts:

CSS selector nesting: we would have to do

[class*="euiBreadcrumb"] { ... } // header-specific styles/overrides [class*="euiBreadcrumb__content"] { ... } // header-specific styles/overrides

Which, if consumers wanted to override those overrides, they would have to do so either via !important or their own nested selector. Not a huge deal, but IIRC we were trying to flatten nested selectors as part of the Emotion conversion.

Developer styling - if I were trying to write new styles for header breadcrumbs and some parent CSS in EuiBreadcrumb was messing with that or causing unexpected effects, it might not be 100% clear where the parent breadcrumb styles live or are coming from (vs being able to skim all styles at once in one file). Again, not a huge or impossible ordeal to surmount.

We can definitely wait for Caroline to get back for clarity. In the interim I pushed up 6051809 to hide the new internal props from the props table, and renamed them more specifically/added comments as to their internal nature

The way I look at it is that no matter how we obfuscate the new prop, consumers can still use EuiHeaderBreadcrumbs anywhere they want. In this PR, the only header-specific styles applied to EuiBreadcrumbs is to enforce truncation and spacing when within the context of EuiHeader. If a consumer wants the look of EuiHeaderBreadcrumbs outside of EuiHeader, they'll most likely be fighting with these extra styles.

Therefore, I suggest keeping this new prop visible to all consumers so they can attain this look anywhere. But I think the name isInEuiHeader is a bit awkward and a pattern we haven't established. I would consider naming it something that points to the purpose of the style and make it an enum, maybe something like type: 'page' | 'application'. And commenting that there should really only be on application style breadcrumb per view.

Sorry, yes, isInEuiHeader is overly explicit/awkward and is only what I would have named it if we weren't going to publicly expose the prop. I like the type prop instead for helping designate the purpose!

a686260 & 734690e - both new props should now be documented and testable in the playground.

@cchaos @miukimiu, do either of you want to do a pass on this before merging? From an engineering review POV, this is good to go.

kibanamachine · 2022-07-14T22:17:17Z

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

cee-chen

Logical CSS properties pass

src/components/header/header_breadcrumbs/header_breadcrumbs.styles.ts

src/components/breadcrumbs/breadcrumbs.styles.ts

1Copenut · 2022-07-20T13:17:34Z

I'll get these values converted to logicalCSS late today and re-up the PR.

kibanamachine · 2022-07-20T18:42:54Z

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

+ clarify prop documentation + simplify style declaration

kibanamachine · 2022-07-20T20:28:15Z

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

- for easier dev readability and testing + remove unnecessary classNames modifiers - they're not used in Kibana anywhere

- pass `maxWidth` to text truncation util - instead of using `> a` or `> span` selectors, just set the styles directly on the content child + use JS to conditionally skip the last breadcrumb - remove `isTruncated` on parent li, it's no longer needed

- they're already handled in the content CSS and incidentally also in EuiLink styles

- apply nested CSS to .euiBreadcrumb__content directly instead of using nested selectors - use a nested object and conditional JS logic (vs first/only/last-child) to flatten and DRY out CSS

- Remove unnecessary `--collapsed` modifier class - DRY out `<li>` and `__content` logic/styles by reusing the `EuiBreadcrumb` and `EuiBreadcrumbContent` components, passing props/overrides as necessary - Allow any children as `EuiBreadcrumb` child to support this behavior/the popover - Pass EuiBreadcrumbContent in at the EuiBreadcrumbs level as a result

- in favor of JS logic instead

- remove unnecessary `--truncate` modifier - not used in Kibana

+ update snapshots

- For easier dev readability / testing + microperf optimization onWindowResize: move declaration to useEffect so it's only declared every time `responsive` changes instead of on every rerender + remove unnecessary conditional around state setter, React already does its own optimization around values that stay the same

- Move to separate util for easier reading / testing - Memoize both limitBreadcrumb and breadcrumb mapping so arrays aren't being traversed on every rerender - refactor: instead of potentially rendering a bunch of hidden breadcrumb elements that never get displayed, splice the breadcrumbs array first, insert a custom object that signifies the EuiBreadcrumbCollapsed button, and then have the element traversal map handle conditionally rendering the collapse button in-place

cee-chen · 2022-07-21T03:15:22Z

@1Copenut The component split was super helpful in mentally organizing and cleaning up a lot of the nested styles. I should have pushed almost all of the refactors I wanted to make (see previous comment about isHeaderBreadcrumb naming, that's probably my only nit left). Let me know what you think!

kibanamachine · 2022-07-21T03:36:19Z

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

- rename both props to be more specific, since they're now internal only / used for a specific purpose - move both props to an underscore prefixed type and modify props table and playground to not display them

kibanamachine · 2022-07-22T18:54:47Z

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

…e` prop

- This may be useful to other consumers where their last breadcrumb is not the current page + tweak styling to show color difference for both `span`s and `a` tags - using JS `color` props instead of CSS + fix `color` prop being passed to underlying span tags

- the `<li>` parent still needs an overflow hidden as it turns out - the last breadcrumb also still needs text truncation styles, it's just max-width should be set ot none

kibanamachine · 2022-07-25T21:51:47Z

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

cee-chen

@1Copenut

kibanamachine · 2022-07-28T17:10:53Z

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

1Copenut added the emotion label May 27, 2022

1Copenut requested a review from cchaos May 27, 2022 00:38

1Copenut self-assigned this May 27, 2022

Tactfully merging main and picking work up again.

6fd7844

1Copenut requested review from elizabetdev and cee-chen July 14, 2022 21:51

1Copenut marked this pull request as ready for review July 14, 2022 21:52

1Copenut commented Jul 14, 2022

View reviewed changes

cee-chen reviewed Jul 19, 2022

View reviewed changes

Apply suggestions from code review

53c6ca2

cee-chen added 2 commits July 20, 2022 11:49

Fix Prettier complaints from GitHub suggestions

4424580

[PR feedback] Rename headerBreadcrumb to use boolean var naming

8d07aa0

+ clarify prop documentation + simplify style declaration

cee-chen added 9 commits July 20, 2022 18:19

Split out internal EuiBreadcrumb & EuiBreadcrumbContent components

4f24e55

- for easier dev readability and testing + remove unnecessary classNames modifiers - they're not used in Kibana anywhere

Simplify text truncation CSS

9cc29e2

- pass `maxWidth` to text truncation util - instead of using `> a` or `> span` selectors, just set the styles directly on the content child + use JS to conditionally skip the last breadcrumb - remove `isTruncated` on parent li, it's no longer needed

Remove unnecessary repeated font-weight declarations

347725a

- they're already handled in the content CSS and incidentally also in EuiLink styles

Simplify and clean up nested .euiBreadcrumb styles

a455722

- apply nested CSS to .euiBreadcrumb__content directly instead of using nested selectors - use a nested object and conditional JS logic (vs first/only/last-child) to flatten and DRY out CSS

Remove inPopover CSS/nesting

e50a5f9

- in favor of JS logic instead

Misc EuiBreadcrumbs CSS cleanup

89be267

- remove unnecessary `--truncate` modifier - not used in Kibana

Fix EuiControlBar Sass styling

bfb5e9b

+ update snapshots

[PR feedback] Hide new header styling and nested/popover props from docs

6051809

- rename both props to be more specific, since they're now internal only / used for a specific purpose - move both props to an underscore prefixed type and modify props table and playground to not display them

cee-chen added 4 commits July 25, 2022 13:28

[PR feedback] Change internal isInEuiHeader prop to an exposed `typ…

a686260

…e` prop

Fix the "Truncate each breadcrumb" example not matching production

92d71fb

- the `<li>` parent still needs an overflow hidden as it turns out - the last breadcrumb also still needs text truncation styles, it's just max-width should be set ot none

Merge branch 'main' into feature/tpierce-breadcrumbs-emotion

9b5687f

cee-chen approved these changes Jul 27, 2022

View reviewed changes

1Copenut and others added 4 commits July 28, 2022 11:22

Merging main to handle breadcrumbs.tsx conflict.

58ecfea

Fix useCurrentEuiBreakpoint refactor lost in merge conflict

5d3de7e

Remove deleted Sass variables from JSON files and document in changelog

0d01636

woops 1 more smol thing

9a90af4

1Copenut merged commit 6ad8245 into elastic:main Jul 28, 2022

1Copenut deleted the feature/tpierce-breadcrumbs-emotion branch July 28, 2022 17:45

thompsongl mentioned this pull request Aug 15, 2022

Upgrade EUI to v62.2.3 elastic/kibana#138837

Merged

tomitank mentioned this pull request Sep 6, 2022

Breadcrumbs and navigation link dont change into popover #6204

Closed

cee-chen mentioned this pull request Sep 30, 2022

[EuiBreadcrumbs] Fix the last breadcrumb in the array not respecting truncate overrides #6280

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving EuiBreadcrumb styles to Emotion. #5934

Moving EuiBreadcrumb styles to Emotion. #5934

1Copenut commented May 27, 2022 •

edited

Loading

kibanamachine commented May 27, 2022

cchaos commented May 27, 2022

kibanamachine commented Jul 13, 2022

1Copenut left a comment

1Copenut Jul 14, 2022

cee-chen Jul 21, 2022

1Copenut Jul 21, 2022

cee-chen Jul 21, 2022 •

edited

Loading

1Copenut Jul 21, 2022

elizabetdev Jul 22, 2022

cee-chen Jul 22, 2022 •

edited

Loading

cchaos Jul 25, 2022

cee-chen Jul 25, 2022

cee-chen Jul 25, 2022

kibanamachine commented Jul 14, 2022

cee-chen left a comment

1Copenut commented Jul 20, 2022

kibanamachine commented Jul 20, 2022

kibanamachine commented Jul 20, 2022

cee-chen commented Jul 21, 2022

kibanamachine commented Jul 21, 2022

kibanamachine commented Jul 22, 2022

kibanamachine commented Jul 25, 2022

cee-chen left a comment

kibanamachine commented Jul 28, 2022

Moving EuiBreadcrumb styles to Emotion. #5934

Moving EuiBreadcrumb styles to Emotion. #5934

Conversation

1Copenut commented May 27, 2022 • edited Loading

Summary

Checklist

kibanamachine commented May 27, 2022

cchaos commented May 27, 2022

kibanamachine commented Jul 13, 2022

1Copenut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Jul 14, 2022

cee-chen left a comment

Choose a reason for hiding this comment

1Copenut commented Jul 20, 2022

kibanamachine commented Jul 20, 2022

kibanamachine commented Jul 20, 2022

cee-chen commented Jul 21, 2022

kibanamachine commented Jul 21, 2022

kibanamachine commented Jul 22, 2022

kibanamachine commented Jul 25, 2022

cee-chen left a comment

Choose a reason for hiding this comment

kibanamachine commented Jul 28, 2022

1Copenut commented May 27, 2022 •

edited

Loading

cee-chen Jul 21, 2022 •

edited

Loading

cee-chen Jul 22, 2022 •

edited

Loading