-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
[SharedUX] EUI visual refresh for SharedUX #202780
[SharedUX] EUI visual refresh for SharedUX #202780
Conversation
282c488
to
9c6ba0d
Compare
.euiAccordion-isOpen & { | ||
color: ${euiTheme.colors.title}; | ||
color: ${euiTheme.colors.textHeading}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How to access this UI:
guide-panel-step.mov
<EuiTabs css={{ padding: `0 ${euiThemeVars.euiSizeL}` }}>{this.renderTabs()}</EuiTabs> | ||
<EuiTabs css={({ euiTheme }) => ({ padding: `0 ${euiTheme.size.l}` })}> | ||
{this.renderTabs()} | ||
</EuiTabs> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accessing this element in Kibana:
tutorial-instruction-set.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might strip this change out of this PR because the Jest test that is impacted is pretty difficult to get updated.
border: `${euiTheme.border.thin}`, | ||
color: `${euiTheme.colors.text}`, | ||
color: `${euiTheme.colors.textParagraph}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accessing this from Kibana:
toolbar-buttons.mov
return ( | ||
<EuiBetaBadge | ||
label={text} | ||
size="s" | ||
css={css` | ||
margin-left: ${euiTheme.size.s}; | ||
color: ${euiTheme.colors.text}; | ||
color: ${euiTheme.colors.textParagraph}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we are actually using this component anywhere currently, but it could be somewhere in the navigation panel for solutions:
navigation-panels.mov
@@ -143,7 +143,7 @@ export const FileUpload = React.forwardRef<EuiFilePickerClass, Props>( | |||
css={css` | |||
display: flex; | |||
align-items: center; | |||
min-height: ${euiButtonHeightSmall}; | |||
min-height: ${euiTheme.size.xl}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this component through the developer examples. To do this, you have to run Kibana with the --run-examples
flag
file-example.mov
c63463d
to
a24e855
Compare
@@ -94,7 +94,7 @@ export const QuitGuideModal = ({ | |||
onClick={deactivateGuide} | |||
isLoading={isLoading} | |||
fill | |||
color="warning" | |||
color="primary" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change request came from @andreadelrio
> | ||
<SideNavComponentLazy {...props} /> | ||
</Suspense> | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just affects the loading state of the solution side nav. You will need to throttle the browser's network or play back a video frame-by-frame to see it.
side-nav-loading-state.mov
@@ -48,7 +48,7 @@ | |||
} | |||
&.securitySolution { | |||
.euiCard__image { | |||
background-color: $euiColorSuccess; | |||
background-color: $euiColorAccentSecondary; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
analytics-overview.mov
abcc004
to
b4798c8
Compare
<EuiFlexItem | ||
css={({ euiTheme }) => ({ marginLeft: `-${euiTheme.size.base}` })} | ||
grow={false} | ||
> | ||
<EuiIconTip type="iInCircle" content={this.props.mustCopyOnSaveMessage} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message was added by #175062. It looks like this code might no longer be reachable, as there seems to be no longer an "Edit" button when viewing managed content (tested with a managed dashboard). There is only a "Duplicate" button.
@@ -72,7 +79,7 @@ export const TagTable: FC<TagTableProps> = ({ | |||
<> | |||
<TagBadge tag={tag} /> | |||
{tag.managed && ( | |||
<div css={{ marginLeft: euiThemeVars.euiSizeS }}> | |||
<div css={{ marginLeft: euiTheme.size.s }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
saved-objects-tagging-management.mov
<SideNavComponentLazy {...props} /> | ||
</Suspense> | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to run Kibana in serverless mode to see this. When Kibana first loads, the side navigation shows a loading icon where the navigation elements appear, similar to https://github.com/elastic/kibana/pull/202780/files#r1870452922
serverless-side-navigation.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Design changes LGTM. Thanks! I will create a follow up issue for the illustration that have hard-coded colors.
The build metrics show large increase in module count and async chunk sizes for the I've pushed 73d9d65 to test if this has a positive impact on the build metrics. Details to come soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
codeowner changes lgtm
@@ -28,7 +28,7 @@ import { | |||
import * as StatusCheckStates from './status_check_states'; | |||
|
|||
import { injectI18n, FormattedMessage } from '@kbn/i18n-react'; | |||
import { euiThemeVars } from '@kbn/ui-theme'; | |||
import { euiThemeVars } from '@kbn/ui-theme'; // FIXME: remove this, and access style variables from EUI context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might be able to be rid of this, if we adopt the css style function callback on L281
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eokoneyo that works fine as a solution to remove this import, but unfortunately that has heavy impact to the Jest unit test for the src/plugins/home/public/application/components/tutorial/tutorial.test.js
file. The test would need to mount the component wrapped in <EuiProvider>
, which changes the structure of the component, and the tests themselves are using Enzyme and are heavily dependent on the structure of the component.
This entire plugin is on the roadmap to be rewritten into TypeScript in the next quarter. Maybe just adding this FIXME comment is the right thing to do for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tsullivan right... it's probably best to resolve this much later then
@@ -58,7 +58,7 @@ export const GuideButton = ({ | |||
<EuiButton | |||
isLoading={isLoading} | |||
onClick={toggleGuidePanel} | |||
color="success" | |||
color="accent" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Is this expected? Or should it rather be accentSecondary
? @andreadelrio
I thought accent
buttons shouldn't really be used? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that we don't know for certain if we will make accentSecondary
an available button given the numerous concerns raised by designers about having two greens (particularly in buttons). Therefore I think it's safer to use the accent
button which has existed in the system for a long time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, yeah I think I was behind on latest button color decisions 😅 I also just saw this (message).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM from EUI side 👍
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
Page load bundle
History
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM, thanks for this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM, thanks for this!
Starting backport for target branches: 8.x |
Backport workflow has been cancelled |
## Summary Part of elastic#200620 1. Remove usage of deprecated color variables 2. Remove usage of `@kbn/ui-theme` 3. A few other changes as requested by @andreadelrio --------- Co-authored-by: kibanamachine <[email protected]>
Summary
Closes #200620
@kbn/ui-theme