Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moving EuiBreadcrumb styles to Emotion. #5934

Merged
merged 25 commits into from
Jul 28, 2022
Merged

Moving EuiBreadcrumb styles to Emotion. #5934

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
62a1154
Moving EuiBreadcrumb styles to Emotion.
1Copenut May 24, 2022
6fd7844
Tactfully merging main and picking work up again.
1Copenut Jul 13, 2022
668ddb5
Adding EUIHeaderBreadcrumb styles.
1Copenut Jul 13, 2022
53c6ca2
Apply suggestions from code review
constancecchen Jul 20, 2022
4424580
Fix Prettier complaints from GitHub suggestions
cee-chen Jul 20, 2022
8d07aa0
[PR feedback] Rename `headerBreadcrumb` to use boolean var naming
cee-chen Jul 20, 2022
4f24e55
Split out internal EuiBreadcrumb & EuiBreadcrumbContent components
cee-chen Jul 20, 2022
9cc29e2
Simplify text truncation CSS
cee-chen Jul 20, 2022
347725a
Remove unnecessary repeated font-weight declarations
cee-chen Jul 20, 2022
a455722
Simplify and clean up nested .euiBreadcrumb styles
cee-chen Jul 20, 2022
8aa914e
Refactor `EuiBreadcrumbCollapsed` into separate component
cee-chen Jul 21, 2022
e50a5f9
Remove `inPopover` CSS/nesting
cee-chen Jul 21, 2022
89be267
Misc EuiBreadcrumbs CSS cleanup
cee-chen Jul 21, 2022
bfb5e9b
Fix EuiControlBar Sass styling
cee-chen Jul 21, 2022
a267e5e
Refactor responsive detection/calculation logic to separate util
cee-chen Jul 21, 2022
63d7bdd
Refactor & optimize limitBreadcrumbs
cee-chen Jul 21, 2022
6051809
[PR feedback] Hide new header styling and nested/popover props from docs
cee-chen Jul 22, 2022
a686260
[PR feedback] Change internal `isInEuiHeader` prop to an exposed `typ…
cee-chen Jul 25, 2022
734690e
Refactor out last internal prop to more descriptive flag
cee-chen Jul 25, 2022
92d71fb
Fix the "Truncate each breadcrumb" example not matching production
cee-chen Jul 25, 2022
9b5687f
Merge branch 'main' into feature/tpierce-breadcrumbs-emotion
cee-chen Jul 25, 2022
58ecfea
Merging main to handle breadcrumbs.tsx conflict.
1Copenut Jul 28, 2022
5d3de7e
Fix useCurrentEuiBreakpoint refactor lost in merge conflict
cee-chen Jul 28, 2022
0d01636
Remove deleted Sass variables from JSON files and document in changelog
cee-chen Jul 28, 2022
9a90af4
woops 1 more smol thing
cee-chen Jul 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
204 changes: 102 additions & 102 deletions src/components/breadcrumbs/__snapshots__/breadcrumbs.test.tsx.snap
View file
Open in desktop

Large diffs are not rendered by default.

79 changes: 0 additions & 79 deletions src/components/breadcrumbs/_breadcrumbs.scss
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions src/components/breadcrumbs/_index.scss
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions src/components/breadcrumbs/_variables.scss
View file
Open in desktop

This file was deleted.

238 changes: 238 additions & 0 deletions src/components/breadcrumbs/breadcrumbs.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,238 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { css } from '@emotion/react';
import { UseEuiTheme } from '../../services';
import { transparentize } from '../../services/color';
import {
euiFontSize,
euiTextTruncate,
euiFocusRing,
} from '../../global_styling/mixins';
cee-chen marked this conversation as resolved.
Show resolved Hide resolved

export const euiBreadcrumbsListStyles = (euiThemeContext: UseEuiTheme) => {
// Styles cast to the <nav> element
const { euiTheme } = euiThemeContext;
return {
euiBreadcrumbs__list: css`
${euiFontSize(euiThemeContext, 's')};
align-items: center;
display: flex;
flex-wrap: wrap;
line-height: ${euiTheme.size.l};
margin-bottom: -${euiTheme.size.xs};
min-width: 0; // Ensure it shrinks if the window is narrow
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
`,
isTruncated: css`
flex-wrap: nowrap;
white-space: nowrap;
`,
};
};

export const euiBreadcrumbStyles = (euiThemeContext: UseEuiTheme) => {
// Styles cast to <li> and descendant elements
const { euiTheme } = euiThemeContext;
return {
euiBreadcrumb: css`
align-items: center;
display: flex;
margin-bottom: ${euiTheme.size.xs}; /* 1 */
cee-chen marked this conversation as resolved.
Show resolved Hide resolved

&:not(:last-of-type) {
color: ${euiTheme.colors.subduedText};

&:after {
background: ${euiTheme.colors.lightShade};
content: '';
flex-shrink: 0;
height: ${euiTheme.size.base};
margin: ${euiTheme.size.xs} ${euiTheme.size.s} 0;
transform: translateY(-1px) rotate(15deg);
width: 1px;
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
}
}

&:last-of-type {
font-weight: ${euiTheme.font.weight.medium};
}
`,
isCollapsed: css`
flex-shrink: 0;

& a,
& button {
:focus {
outline-offset: -1px;
}
}
`,
isTruncated: css`
overflow: hidden;

&:not([class*='euiBreadcrumb-isCollapsed']) {
& > a,
& > span {
max-width: calc(${euiTheme.size.base} * 10);
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
overflow: hidden;
text-overflow: ellipsis;
}
}

&:last-of-type {
& > a,
& > span {
max-width: none;
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
}
}
`,
isHeaderBreadcrumb: css`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.

  2. 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

@cee-chen cee-chen Jul 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

screencap

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😄

Copy link
Contributor

@cee-chen cee-chen Jul 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.

  1. 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

screencap

@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.

&:first-child {
& > a,
& > [class*='euiPopover'] button,
& > span {
border-radius: ${euiTheme.border.radius.medium} 0 0
${euiTheme.border.radius.medium};
clip-path: polygon(
0 0,
calc(100% - ${euiTheme.size.s}) 0,
100% 50%,
calc(100% - ${euiTheme.size.s}) 100%,
0 100%
);
padding-left: ${euiTheme.size.m};
}
}

&:only-child {
& > a,
& > span {
border-radius: ${euiTheme.border.radius.medium};
clip-path: none;
padding-left: ${euiTheme.size.m};
padding-right: ${euiTheme.size.m};
}
}

&:last-child {
& > a,
& > span {
border-radius: 0 ${euiTheme.border.radius.medium}
${euiTheme.border.radius.medium} 0;
clip-path: polygon(
0 0,
100% 0,
100% 100%,
0 100%,
${euiTheme.size.s} 50%
);
padding-right: ${euiTheme.size.m};
}
}

&:not(:last-of-type) {
margin-right: -${euiTheme.size.xs};
}

&::after {
display: none;
}

> [class*='euiPopover'] button {
${euiFontSize(euiThemeContext, 'xs')};
background-color: ${transparentize(euiTheme.colors.primary, 0.2)};
color: ${euiTheme.colors.link};
clip-path: polygon(
0 0,
calc(100% - ${euiTheme.size.s}) 0,
100% 50%,
calc(100% - ${euiTheme.size.s}) 100%,
0 100%,
${euiTheme.size.s} 50%
);
font-weight: ${euiTheme.font.weight.medium};
line-height: ${euiTheme.size.base};
padding: ${euiTheme.size.xs} ${euiTheme.size.base};

:focus {
${euiFocusRing(euiTheme, 'inset', {
color: `${euiTheme.colors.link}`,
})};
color: ${euiTheme.colors.link};

:focus-visible {
border-radius: ${euiTheme.border.radius.medium};
clip-path: none;
}
}
}
`,
};
};

export const euiBreadcrumbContentStyles = (euiThemeContext: UseEuiTheme) => {
// Styles cast to <a> or <span> elements
const { euiTheme } = euiThemeContext;
return {
euiBreadcrumb__content: css`
font-weight: ${euiTheme.font.weight.medium};

&:is(a) {
:focus {
${euiFocusRing(euiTheme, 'inset')};
}
}
`,
isTruncated: css`
${euiTextTruncate()};
max-width: calc(${euiTheme.size.base} * 10);
text-align: center;
vertical-align: baseline;
`,
isHeaderBreadcrumb: css`
${euiFontSize(euiThemeContext, 'xs')};
background-color: ${transparentize(euiTheme.colors.darkestShade, 0.2)};
clip-path: polygon(
0 0,
calc(100% - ${euiTheme.size.s}) 0,
100% 50%,
calc(100% - ${euiTheme.size.s}) 100%,
0 100%,
${euiTheme.size.s} 50%
);
color: ${euiTheme.colors.darkestShade};
font-weight: ${euiTheme.font.weight.medium};
line-height: ${euiTheme.size.base};
padding: ${euiTheme.size.xs} ${euiTheme.size.base};
cee-chen marked this conversation as resolved.
Show resolved Hide resolved

&:is(a) {
background-color: ${transparentize(euiTheme.colors.primary, 0.2)};
color: ${euiTheme.colors.link};

:focus {
${euiFocusRing(euiTheme, 'inset')};

:focus-visible {
border-radius: ${euiTheme.border.radius.medium};
clip-path: none;
}
}
}
`,
};
};

export const euiBreadcrumbsInPopoverStyles = ({ euiTheme }: UseEuiTheme) => ({
// Styles cast to breadcrumbs in EUI Popover
euiBreadcrumbs__inPopover: css`
& li:last-of-type > a,
& li:last-of-type > span {
color: ${euiTheme.colors.subduedText};
}
`,
});
Loading