-
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
[Shared UX] Move Solution Toolbar components to package #134392
[Shared UX] Move Solution Toolbar components to package #134392
Conversation
As in the |
I would be descriptive about what it is: |
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.
Found a few lingering mentions of "Solution"
packages/shared-ux/button_toolbar/src/buttons/add_from_library/add_from_library.stories.tsx
Outdated
Show resolved
Hide resolved
packages/shared-ux/button_toolbar/src/buttons/icon_button_group/icon_button_group.stories.tsx
Outdated
Show resolved
Hide resolved
packages/shared-ux/button_toolbar/src/buttons/icon_button_group/icon_button_group.stories.tsx
Outdated
Show resolved
Hide resolved
packages/shared-ux/button_toolbar/src/buttons/primary/primary.stories.tsx
Outdated
Show resolved
Hide resolved
packages/shared-ux/button_toolbar/src/popover/popover.stories.tsx
Outdated
Show resolved
Hide resolved
packages/shared-ux/button_toolbar/src/popover/popover.stories.tsx
Outdated
Show resolved
Hide resolved
@thompsongl You can see the issue here: https://ci-artifacts.kibana.dev/storybooks/pr-134392/f0093ba142591a6e8320a9daba00e05a5f3fa28c/shared_ux/index.html?path=/story/solution-toolbar-buttons--icon-button-group The styles are overridden by the base EUI styles. Curious how to adapt the Emotion styles to overcome that specificity. And this used to work, AFAIK. |
Co-authored-by: Caroline Horn <[email protected]>
…-ref HEAD~1..HEAD --fix'
Spoke to @clintandrewhall about the specificity problem and it's a known effect. The EUI team has a fix in the works that will enable Kibana to better order styles coming from Emotion: elastic/eui#5853 This will be available and enabled in Kibana by 8.4 FF. You'll need to do no extra work. If you want to increase specificity in the meantime, you can chain the selector in question: |
Thanks @thompsongl ... we'll just leave as-is, for now, and let your fix take care of it. |
Are the any actual usages of this right now? I would be concerned about this visual regression even if temporary. But if it's not consumed just yet, then we're ok. |
@cchaos yeah, it's not used anywhere yet |
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.
Presentation team changes LGTM, just changes to deprecation comments.
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Unknown metric groupsAPI count
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.
Tested in storybook in Chrome on Mac OSX, works as expected. Had only a brief look at the code, since it's mostly moving files around, I wasn't expecting any major changes.
Summary
As titled, moving another set of components to a package.
Open Questions
SolutionToolbar
yet.IconButtonGroup
is using anemotion
technique I'm not familiar with, and it's also not working, (the styles are being overloaded by specificity). Can you assist with a technique to be more current that also works?