-
Notifications
You must be signed in to change notification settings - Fork 81
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
Feature: Add checkbox buttons (NcCheckboxRadioSwitch buttons that look like NcButtons) #4280
Conversation
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.
Looks good! But should we show so many possible options, like Primary, Secondary, Tertiary, Tertiary without background?
Only secondary and tertiary are really used, are they? @szaimen @marcoambrosini
Thank you :)
I just took all button styles we provide for |
If we expose the button with text only as a checkbox button shouldn't it then have a visually clear indicator that it is in a checked state? |
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.
Instead of adding a new pattern just for this, can't we do the same as we decided for the text tool bar and switch the favourite items from secondary to primary? This seems to me like an analogue use case?
60a0666
to
d16ef31
Compare
Good point, but how would this look like? Adding a border? BTW: We could also make the font bold if checked, but I think the button will then resize on click. |
What do you mean with a new pattern? For the first change the idea was that we already have button styles of the checkboxes, but they do not look like buttons, so for consistency they should look like |
By "pattern" I mean a new way to solve this problem. Since we decided to switch to primary for "selected" or "activated" looking buttons elsewhere, I would stick to that everywhere else too.
Could you point me to places where we have this? @nextcloud/designers what do you think? |
Sound good, is there a styleguide for this somewhere? Because I like the idea but have this questions:
You can already use |
This is not documented yet as we took this decision rather recently. The activated state is always primary as it needs to meet certain contrast requirements that the secondary button doesn't meet. And yes it's possible to have a tertiary and activating it by making it primary, that's exactly what we're doing in the text toolbar :) |
If we implement this behaviour in this PR here, I think that would be the ideal solution (meaning not allowing all kinds of button types, only primary for |
…te stateful button Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
…r accessibility Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
d16ef31
to
7686227
Compare
@marcoambrosini Ok I now changed it to not allow primary and only use it for the enabled state, what do you think? vokoscreenNG-2023-07-04_21-23-56.mp4 |
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 think this should be within the checkboxradioswitch
component. This component already has a lot of use cases and many different styles associated to it. And more importantly, these are not checkboxes, they're still buttons. @raimund-schluessler sorry, I misread your comment before: these will not be used inside a form and will not require any further action to make the changes effective. This is just 1 action that just gets performed immediately. Just like in the action menu we add to favourites with an action button, I think we should use plain buttons here.
I think that the best way to go is to just add a prop to the button component to "activate" it and force primary when we do so.
Well the primary issue why I created this PR is that a normal button is not the correct element to use for having a state. Because changing it to |
We can add the This is also how google does it in their docs toolbars and I think it makes a lot of sense for the text toolbar and the favourite button too. Maybe we can call the button prop Screen.Recording.2023-07-05.at.18.46.55.mov |
Do we have an update on this? :) Since accessibility issues are quite important to fix, it’s often good to go with the most pragmatic solution first and refine in iterations. |
@jancborchardt I can add the |
If we use the "pressed" option on the NcButton, we maybe want to disable the button styling on |
Superseded by #4344 |
Could you please point me to some examples of where that's used @susnux? |
☑️ Resolves
This allows to create checkbox buttons -> Make
NcCheckboxRadioSwitch
with button variant look likeNcButton
.Checkbox buttons are required when you use buttons to toggle a state, e.g. the favorite "star" button on the app sidebar is a button but only shows its state visually. For a11y the button should be implemented as a checkbox instead.
🖼️ Screenshots
Checkbox buttons
vokoscreenNG-2023-06-27_13-29-40.mp4
AppSidebar favorite button
🏁 Checklist