-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Design update for extract examples #42399
Conversation
Codenotify: Notifying subscribers in CODENOTIFY files for diff e23e654...479dba7.
|
|
2d53129
to
1f04efc
Compare
width: 100%; | ||
} | ||
|
||
.label { |
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 is one of the cases I discussed in the FE guild.
I need the button content to render as flex in this one usecase, do I need to override all Buttons everywhere on the site for that?
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 the same as https://metaboat.slack.com/archives/C057WD5L0JG/p1715204376544389?
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.
It's a similar issue than in that thread, but not the same exact component.
ClickActionControl
is using some deprecated components and we're adding new designs and features to it.
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.
But you're overrides styles for a button in metabase/ui, which is new
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.
True, I meant more that this seems like a very one of usecase for the button.
I'm happy to make it a custom button, so there are no overrides, but I feel like that's not the way to go either.
The only reason I switched to ui/Button
was because the legacy button that was used could not be customized properly (without having a large blast radius).
But I understand the comment to be about making sure we streamline the design process into forcing a more principled approach.
I'm not sure what the next steps here should be: push back to the design team? Use a fully custom component instead of using overrides?
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.
Could you ask @kdoh in ui proj channel?
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.
Unclear style override
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 don't want to block this but something is clearly going wrong here
Closes #42375
Description
Render the examples for Extract Column inline instead of underneath the title.
How to verify
Describe the steps to verify that the changes are working as expected.
Extract column
Demo
Notes
I updated the horizontal
ClickActionsControl
to useButton
frommetabase/ui
for this, instead of the deprecatedButton
. I can convert the rest to use CSS modules too if that is preferred.