-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[SharedUX] EUI visual refresh for SharedUX #202780
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
Conversation
282c488 to
9c6ba0d
Compare
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
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.
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
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
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
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
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
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
| 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.
| <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
andreadelrio
left a comment
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. |
mattkime
left a comment
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
|
|
||
| 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
| 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).
mgadewoll
left a comment
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
|
eokoneyo
left a comment
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!
eokoneyo
left a comment
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 https://github.com/elastic/kibana/actions/runs/12303034965 |
|
Backport workflow has been cancelled |
Summary
Closes #200620
@kbn/ui-theme