Fix tabs appearance #4815
Fix tabs appearance #4815
Conversation
marcoambrosini
added
enhancement
Signed-off-by: Marco <marcoambrosini@icloud.com>
marcoambrosini
force-pushed
the
enh/improve-appsidebartabs-look
branch
from
5afe437
to
a72bddd
Compare
marcoambrosini
requested review from
skjnldsv,
susnux,
raimund-schluessler and
szaimen
|
So do we want to go back with the design? This was basically the reason we needed to go with a new major release. |
|
Why do we go back now? The design feedback in nextcloud/server#41450 does not say anything about this, just mentions a small fix. |
| :deep(.checkbox-radio-switch--button-variant.checkbox-radio-switch) { | ||
| border: unset; | ||
| // Override checkbox-radio-switch styles so that it looks like tabs | ||
| :deep(.checkbox-radio-switch--button-variant) { |
There was a problem hiding this comment.
This completely changes the UI of the component, should we instead change that component?
Because this is now very likely to break on slight changes of the internals of NcCheckboxRadioSwitch.
For the same reason I feel quite uncomfortable with that the many !important statements.
There was a problem hiding this comment.
Could we first learn what the intended goal is here? Do we intend to go back with the design completely? Is it just about small fixes?
There was a problem hiding this comment.
There has been a last minute request to revert this, sorry. It's a last minute fix that concerns only the sidebar tabs, hence the deep and important. It's temporary, not permanent. We can discuss what to do with checkboxradioswitch later on.
There was a problem hiding this comment.
That said, I think we're trying to do way too many things with NcCheckboxRadioSwitch
There was a problem hiding this comment.
@marcoambrosini can you make sure it doesn’t break in these 2 places:
- Sharing flow where we use the component vertically (right?)
- Forms on top right, where we use it for View/Edit/Results
There was a problem hiding this comment.
Could we first learn what the intended goal is here? Do we intend to go back with the design completely? Is it just about small fixes?
@raimund-schluessler Yes to both: It is about small fixes, and the active tab being way too present has been identified as a visual issue. Going to a middle ground between the old and new one seems a good way to go.
There was a problem hiding this comment.
Are we sure this won't cause a11y issues? Cc @JuliaKirschenheuter
There was a problem hiding this comment.
Are we sure this won't cause a11y issues? Cc @JuliaKirschenheuter
It only changes styles, so the only possible a11y issues could be about state styles and contrast. With the contrast everything is fine.
But with states it is not.
The current tab should have hover and active state styles.
| Before | After |
|---|---|
 |
 |
@raimund-schluessler the concrete issue is that it’s much too present and makes everything more busy. The "primary" style is ok for the primary action buttons, and it’s also good for the left navigation, but also having this in the right sidebar makes it a bit much. It’s a classic case of "if everything is important, nothing is". :) Does that explain it? |
Thanks for the explanation. I just wonder whether adjusting the color would be enough then, instead of going back to the old design and potentially breaking quite some things. E.g. one could use a similar style to a secondary button:
|
This was an intermediate solution, but did not work correctly: |
@raimund-schluessler as @nickvergessen mentioned this was actually not accessible, reason being that there was too little contrast between an active entry and a non-active entry. They were not distinguishable. The bar below on this design achieves that. |
Hm, I guess we are stuck with the old, and now inconsistent, design then, since no one has a better solution 😕 |
|
As was mentioned in the old PR (#3945 (comment)), from the design perspective, I'd prefer to go with this PR's design. IMO, the radio button switch doesn't look like a tab but presents a tab. I also agree with @susnux that it should not be implemented by deep override of the radio switch. However, I think we can merge it to have the new design soon, and fix the implementation later. |
|
@ShGKme since Marco is probably already out of office since he is on Japan time, could I ask you to add a hover style there which fits accessibility requirements? |
Asked @JuliaKirschenheuter to check as she also worked on this problem with the previous tabs state styles. |
Effects from the link:
|
@jancborchardt could we create same styles for our Tabs? |
|
Note, in the w3 example, they use the same styles as used on keyboard navigation. So in this PR it would look like this. But better to have a gap between the current tab bottom border and the outline.
|
|
@ShGKme @JuliaKirschenheuter I have doubts about the need of this outline for the active state, let alone any hover effects. We've been told before that no hover effects are ever necessary for accessibility. |
There was a problem hiding this comment.
@ShGKme @JuliaKirschenheuter I have doubts about the need of this outline for the active state, let alone any hover effects. We've been told before that no hover effects are ever necessary for accessibility. The link you posted contains one accessible solution, not the only accessible solution.
Without active state, it is not visible if this component is under keyboard navigation right now or not.
For example, I see this state:
If I press Right, will I move to another tab or not? It is unknown because the user doesn't know if the tab button is active or not.
Probably I need to click on the element first. But there is also no hover effect to show that it is clickable (not about a11y).
But let's merge and discuss/fix changes later to have this earlier before the release.
|
Honestly, I feel like this change is a bit drastic and rushed, given that the problem to solve is the tab active being "much too present and makes everything more busy" (basically just an attention issue). With the changes proposed we will introduce hover and active state issues, get an inconsistent design, probably conflicts with the checkbox-radio-switch component, and maybe more. And all this just to fix an attention issue (not even a11y, since this is fine, as far as I understand it). We have other places in the design where we use primary as a background, e.g. the active navigation entry |
Agree! But our light gray color like on hover won't help at all. If this is a requirement then it have to be solved differently. I can't find requirements regarding active styles here https://www.w3.org/WAI/ARIA/apg/patterns/tabs/. And would say we can merge it like it is for now. |
☑️ Resolves
🖼️ Screenshots
🚧 Tasks
🏁 Checklist