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

[Enterprise Search] Fix Kibana beta notification nav CSS overrides leaking into other plugins #107973

Merged
merged 1 commit into from
Aug 9, 2021
Merged

[Enterprise Search] Fix Kibana beta notification nav CSS overrides leaking into other plugins #107973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 12 additions & 9 deletions x-pack/plugins/enterprise_search/public/applications/shared/layout/beta.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,18 @@
*/

@include euiBreakpoint('m', 'l', 'xl') {
.kbnPageTemplateSolutionNav {
display: flex;
flex-direction: column;
}
.euiSideNav__content {
flex-grow: 1;
display: flex;
flex-direction: column;
justify-content: space-between;
.betaSidebarNotification {
.euiSideNav {
display: flex;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same CSS properties as before, just indented. I recommend hiding whitespace changes on the diff.

I changed this from .kbnPageTemplateSolutionNav to .euiSideNav for clarity, we could hook into either class but it made the most sense for me to use .euiSideNav to match .euiSideNav__content later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI, it's never really a good idea to target .eui classes, because we make changes these a bunch including the DOM structure. We don't usually consider them breaking changes since it's not a way we support custom styling.

It may be better to use absolute positioning in this case, or to request a customization option from the component itself. Even could do it in KibanaSolutionNav, where it just accepts children placed after the EuiSideNav component.

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 used absolute positioning originally (#104763), but that broke when the PR to collapse the sidebar landed. Definitely understood that it's not ideal to target EUI classes and we normally don't, but in this case since this is a temporary notification and will be removed in 7.16, I'm honestly not super worried about it and I don't think it needs to be semantically perfect since it has a specific (and fairly short) end of life. We'll definitely keep an eye out for any layout changes or issues that come up between now and 7.16!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, no problem. Mainly knowledge sharing. Also probably a good idea to put in a specific GH issue for removing it (if one doesn't exist yet) and targeting 7.16.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome call, thanks Caroline! I've opened up a draft PR for 7.16 (#108085) as well as creating an issue in our team repo (https://github.com/elastic/app-search-team/issues/1986).

Copy link
Contributor

Choose a reason for hiding this comment

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

You win!

flex-direction: column;

.euiSideNav__content {
flex-grow: 1;
display: flex;
flex-direction: column;
justify-content: space-between;
}
}
}
}

Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/enterprise_search/public/applications/shared/layout/page_template.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ export const EnterpriseSearchPageTemplate: React.FC<PageTemplateProps> = ({
}}
isEmptyState={isEmptyState && !isLoading}
solutionNav={solutionNav ? { icon: 'logoEnterpriseSearch', ...solutionNav } : undefined}
pageSideBarProps={{ className: 'betaSidebarNotification' }}
>
{setPageChrome}
{readOnlyMode && (
Expand Down