-
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
[Canvas] Fix sharable runtime styles and remove color="ghost"
where unnecessary
#169313
Conversation
Pinging @elastic/eui-team (EUI) |
Pinging @elastic/kibana-presentation (Team:Presentation) |
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.
Testing the Canvas shareable runtime requires a few steps.
yarn kbn bootstrap
node x-pack/plugins/canvas/scripts/shareable_runtime.js
yarn start
- Install sample data ecommerce
- Open the "[eCommerce] Revenue Tracking" workpad
- Click "Share - Share on a website"
- In the flyout, click the "download an example ZIP file" link.
- Extract the zip file to a directory
- Inside the directory, run
python -m http.server
and open http://localhost:8000
Any changes to the code require a rebuild of the runtime in step 2. You could copy the output kbn_canvas.js
file to the unzipped directory to test.
The shareable runtime has loads of CSS issues in main
already. Some of the .scss
files use the composes:
selector from CSS-Modules, but I believe we no longer support CSS-Modules in favor of Emotion 🤔 .
Compare to this screenshot from the original PR.
With our current priorities, I do not foresee us spending time fixing the aesthetic CSS issues in the shareable runtime. The shareable runtime is not widely used and we have higher impact features in our roadmap. cc @ThomThomson @cqliu1 in case they feel differently.
Since this PR as it doesn't break anything and it allows EUI to remove the deprecated prop, I'm going to say "lgtm".
@nickpeihl agreed that we should not spend time fixing these aesthetic problems with the shareable runtime. If an engineer from another team wanted to fix this I wouldn't be against it, but it does have very little usage. |
Ahh interesting. Yeah it looks like the bottom bar totally broke during EuiBottomBar's conversion to Emotion (elastic/eui#5823), which removed any Sass styles associated with kibana/x-pack/plugins/canvas/shareable_runtime/components/footer/footer.module.scss Line 9 in 604409b
If there's a quick and easy way to fix |
- now that an `EuiBottomBar` component is being used, and is correctly setting dark mode on all children
That wasn't too bad at all! Here's the fixed styles: Quick note - I'm aware the EuiContextMenu colors are broken - this is an issue to fix on EUI's side of things (see #169571 as well). Once we fix it in EUI, this package should automatically inherit the fix. |
aac0e6a
to
2184c6e
Compare
Amazing, thank you for fixing this @cee-chen! |
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
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.
Thank you @cee-chen for fixing this! Updated changes lgtm!
color="ghost"
button to dark mode wrappercolor="ghost"
where unnecessary
Summary
👋 Hey y'all - EUI will shortly be deprecating the
ghost
color in all button components (see https://eui.elastic.co/v89.0.0/#/navigation/button#ghost-vs-dark-mode).I'm opening this PR ahead of time for your team so you can test this migration and ensure no UI regressions have occurred as a result. To be honest, I'm very unsure how to test the shereable runtime feature - I tried downloading a ZIP file but it emitted a html file that rendered nothing, so I'd appreciate help QAing this if possible.
Checklist