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

[Bug] Fix Global Breadcrumb Styling in dark mode #2085

Merged
merged 5 commits into from
Aug 8, 2022
Merged

[Bug] Fix Global Breadcrumb Styling in dark mode #2085

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
646d09a
fix breadcrumb in dark mode
kaddy645 Aug 5, 2022
f67935a
update snapshot
kaddy645 Aug 5, 2022
4bd021d
update unit test
kaddy645 Aug 5, 2022
8224cb2
pr update
kaddy645 Aug 5, 2022
8520299
remove focus bg
kaddy645 Aug 8, 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
4 changes: 4 additions & 0 deletions src/core/public/_variables.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,7 @@ $osdHeaderOffset: $euiHeaderHeightCompensation;
$osdHeaderBreadcrumbBlueBackground: #b9d9eb;
$osdHeaderBreadcrumbGrayBackground: #d9e1e2;
$osdHeaderBreadcrumbCollapsedLink: #002a3a;
$osdHeaderBreadcrumbPacificSkyDarkestBackground: #163f66;
Copy link
Member

Choose a reason for hiding this comment

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

Was there a style guide approved color that we are using for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

most of these colors are not part of EUI/OUI. these are new colors and eventually will be part of OUI theming. we will be removing these colors once we start working on OUI and breadcrumb will be part of OUI

$osdHeaderBreadcrumbMidnightSkyMediumBackground: #4c636f;
$osdHeaderBreadcrumbMidnightSkyMediumLightColor: #96a0a5;
$osdHeaderBreadcrumbMidnightSkyMediumLightHoverColor: #b0b8bb;
2 changes: 2 additions & 0 deletions src/core/public/chrome/ui/header/__snapshots__/header.test.tsx.snap
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion src/core/public/chrome/ui/header/header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ export function Header({
const toggleCollapsibleNavRef = createRef<HTMLButtonElement & { euiAnimate: () => void }>();
const navId = htmlIdGenerator()();
const className = classnames('hide-for-sharing', 'headerGlobalNav');
const { useExpandedHeader = true } = branding;
const { useExpandedHeader = true, darkMode } = branding;

return (
<>
Expand Down Expand Up @@ -194,6 +194,7 @@ export function Header({
<HeaderBreadcrumbs
appTitle$={observables.appTitle$}
breadcrumbs$={observables.breadcrumbs$}
isDarkMode={darkMode}
/>

<EuiHeaderSectionItem border="none">
Expand Down
78 changes: 60 additions & 18 deletions src/core/public/chrome/ui/header/header_breadcrumbs.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,43 +13,85 @@ $breadcrumbPolygon: polygon(
0% 100%
);

.osdHeaderBreadcrumbs {
/*
filter defines a custom filter effect by grouping atomic filter primitives!
here we are using Gaussian filter with stdDeviation for applying
border-radius on clipped background.
*/

filter: url("../../public/assets/round_filter.svg#round");
/* remove background color on autofocus for euiBreadcrumbs in Popover */
.euiBreadcrumbs__inPopover {
.euiLink.euiLink--text:focus {
background: none;
}
}
kaddy645 marked this conversation as resolved.
Show resolved Hide resolved

.osdBreadcrumbs {
.osdHeaderBreadcrumbs {
/* common for both light & dark theme */
.osdBreadcrumbs,
&--dark .osdBreadcrumbs {
clip-path: $breadcrumbPolygon;
background-color: $osdHeaderBreadcrumbGrayBackground;
padding: $euiSizeXS - 2.5 $euiSizeL - $euiSizeXS;

&:first-child {
clip-path: $firstBreadcrumbPolygon;
}
}

.osdBreadcrumbs {
background-color: $osdHeaderBreadcrumbGrayBackground;

&:last-child {
background-color: $osdHeaderBreadcrumbBlueBackground;
}
}

.euiBreadcrumbSeparator {
display: none;
}

/* only light mode */
kaddy645 marked this conversation as resolved.
Show resolved Hide resolved
.euiBreadcrumb__collapsedLink {
color: $osdHeaderBreadcrumbCollapsedLink;
background: $euiColorEmptyShade;
}

.euiPopover__anchor {
padding: 0 $euiSizeS;
/* common for dark & light mode */
&,
&--dark {
/*
filter defines a custom filter effect by grouping atomic filter primitives!
here we are using Gaussian filter with stdDeviation for applying
border-radius on clipped background.
*/
filter: url("../../public/assets/round_filter.svg#round");

button {
line-height: inherit;
}

.euiBreadcrumbSeparator {
display: none;
}

.euiPopover__anchor {
padding: 0 $euiSizeS;
}

.euiBreadcrumb:not(.euiBreadcrumb:last-child) {
margin-right: 0;
}
}

.euiBreadcrumb:not(.euiBreadcrumb:last-child) {
margin-right: 0;
/* only dark mode */
&--dark {
.osdBreadcrumbs {
background-color: $osdHeaderBreadcrumbMidnightSkyMediumBackground;
color: $osdHeaderBreadcrumbMidnightSkyMediumLightColor;

&:hover {
color: $osdHeaderBreadcrumbMidnightSkyMediumLightHoverColor;
}

&:last-child {
background-color: $osdHeaderBreadcrumbPacificSkyDarkestBackground;
color: $euiColorFullShade;
}
}

.euiBreadcrumb__collapsedLink {
color: $euiColorGhost;
background: $euiColorEmptyShade;
}
kaddy645 marked this conversation as resolved.
Show resolved Hide resolved
}
}
6 changes: 5 additions & 1 deletion src/core/public/chrome/ui/header/header_breadcrumbs.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,11 @@ describe('HeaderBreadcrumbs', () => {
it('renders updates to the breadcrumbs$ observable', () => {
const breadcrumbs$ = new BehaviorSubject([{ text: 'First' }]);
const wrapper = mount(
<HeaderBreadcrumbs appTitle$={new BehaviorSubject('')} breadcrumbs$={breadcrumbs$} />
<HeaderBreadcrumbs
appTitle$={new BehaviorSubject('')}
breadcrumbs$={breadcrumbs$}
isDarkMode={false}
/>
);
expect(wrapper.find('.euiBreadcrumb')).toMatchSnapshot();

Expand Down
6 changes: 4 additions & 2 deletions src/core/public/chrome/ui/header/header_breadcrumbs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,13 @@ import './header_breadcrumbs.scss';
interface Props {
appTitle$: Observable<string>;
breadcrumbs$: Observable<ChromeBreadcrumb[]>;
isDarkMode?: boolean;
}

export function HeaderBreadcrumbs({ appTitle$, breadcrumbs$ }: Props) {
export function HeaderBreadcrumbs({ appTitle$, breadcrumbs$, isDarkMode }: Props) {
const appTitle = useObservable(appTitle$, 'OpenSearch Dashboards');
const breadcrumbs = useObservable(breadcrumbs$, []);
const className = isDarkMode ? 'osdHeaderBreadcrumbs--dark' : 'osdHeaderBreadcrumbs';
let crumbs = breadcrumbs;

if (breadcrumbs.length === 0 && appTitle) {
Expand All @@ -67,7 +69,7 @@ export function HeaderBreadcrumbs({ appTitle$, breadcrumbs$ }: Props) {
breadcrumbs={crumbs}
max={10}
data-test-subj="breadcrumbs"
className="osdHeaderBreadcrumbs"
Copy link
Member

Choose a reason for hiding this comment

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

I can understand for the shape but looking at the original implementation could the theming being retrieved and applied by the shared ui dep package and applied as a prop to the existing bread crumb. That way the shape is handled in our repo but we rely on the theme from OUI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have not made any changes to OUI yet! this whole css/style issue will be part of OUI in future! we will have separate ticket for that

Copy link
Member

@kavilla kavilla Aug 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah sorry about the confusion. What I mean is how come the bread crumb class that we've added has it's own dark mode css instead of pulling the theme from OUI (previously EUI) via the shared ui dep package? Seems like that is the standard within the repo. And then the only new CSS in this repo that we added was with the shape.

Copy link
Contributor Author

@kaddy645 kaddy645 Aug 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I thought these dark mode changes will be part of OUI theming in future but at the moment they are not. so we have diffs css! I will confirm with ux team once! Is it right @kgcreative ?

Copy link
Member

Choose a reason for hiding this comment

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

I'm probably not the best person to talk about CSS implementation details. I would follow whatever guidance we have from a system perspective. The long term plan is to fold these into the updated OUI theme, but we're a long way from that.

className={className}
/>
);
}