-
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][Labs] Integrate Labs Service into Canvas #96920
Conversation
Pinging @elastic/kibana-presentation (Team:Presentation) |
@elasticmachine merge upstream |
@clintandrewhall just so that I'm clear, we're checking the interaction and UI, but the actual changing of the toolbar is not all hooked up, correct? (i.e. I don't see the toolbar change after refresh) I'll keep poking at it, but I generally like the menu link approach you've taken here and the badge/count is nice. |
@ryankeairns That's correct, there's no actual project at the moment, it's just there more as a placeholder. This PR is all about the UI and interaction... we'll create real projects shortly thereafter. |
@clintandrewhall here's a quick design PR with some minor adjustments that align closer to other parts of Kibana. I probably missed an i18n translation 😬 Project list item (top) / aligns to Advanced settings design (bottom)Close link added to footer |
Thank you @ryankeairns !! |
Align design to adv settings, copy updates
@elasticmachine merge upstream |
@clintandrewhall regarding the text to describe what Labs is - I would place that in the flyout header just below the Labs projects title. This seems like a natural fit/is a common pattern and means a single menu click still opens the flyout. It would look something like this... ...and can be achieved doing 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.
I love it! I tested this out locally and couldn't find any issues
@elasticmachine merge upstream |
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.
LGTM 👍 I've just got a couple questions that don't block merging this PR.
Should we remove the unified toolbar setting for now and then reintroduce it when my Canvas toolbar stuff goes in?
Do we want to move this Labs
button to the global nav along with the other WorkpadHeader
buttons once I move that stuff to the global nav?
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.
Telemetry changes LGTM!
@elasticmachine merge upstream |
Yeah, I'll remove it. I left it there for testing purposes.
Yes indeed! |
@elasticmachine merge upstream |
@ryankeairns 🚀 Opted to avoid the link, for now, since each solution will have a different advanced setting to show/hide Labs projects... we can do something more robust if it's adopted more broadly. |
f17fbce
to
4503c52
Compare
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: |
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Co-authored-by: Clint Andrew Hall <[email protected]>
Summary
This PR builds on #95435 by:
React.lazy
loader.I fully expect @ryankeairns and @poffdeluxe to have some feedback/improvements around the menu item. I considered having a menu popover with some explanation text, a button to open labs, etc... perhaps even a small table of which projects have an override? Pulls accepted!
We'll also need to decide whether this change should be backported to 7.13 or planned for 7.14.