Improve `NcCheckboxRadioSwitch` and use for `NcAppSidebarTabs` #3945

susnux · 2023-04-01T01:14:14Z

Resolves Sidebar tabbed navigation normal / hover / focus / active states. #603

I split this into separate commits to make reviewing easier as the PR is quite huge.
The first part adjusts the styling of the checkbox radio switch to be used with for the sidebar tabs (adjust borders and add an icon slot).

Part 1: Improve `NcCheckboxRadioSwitch` styling

Before

The hover colors are confusing, primary color is applied to multiple elements, the border is only applied to not-selected elements.

vokoscreenNG-2023-03-31_22-40-42.mp4

With improvements (added borders)

The border is applied around the whole group, the hover color is fixed to be primary on the checked element and default hover color otherwise.

vokoscreenNG-2023-03-31_23-07-24.mp4

With improvements

The border is applied around the whole group, the hover color is fixed to be primary on the checked element and default hover color otherwise.
Added rounded borders (last commit) to match the mockup in the linked issue.

vokoscreenNG-2023-04-01_02-40-39.mp4

Part 2: Use `NcCheckboxRadioSwitch` for `NcAppSidebarTabs`

before	after

Outdated initial version:

before	after

vokoscreenNG-2023-04-01_03-04-05.mp4

jancborchardt

Looks much much better!

@susnux a question: On part 1 "with improvements, should the single-line radio buttons not have more border-radius so they are pill-style? (Like we do for the View switcher in Forms currently.)

susnux · 2023-04-03T10:02:08Z

@jancborchardt

a question: On part 1 "with improvements, should the single-line radio buttons not have more border-radius so they are pill-style? (Like we do for the View switcher in Forms currently.)

I used your and @nimishavijay comment here as a reference for how it should look like: #603 (comment)

jancborchardt · 2023-04-03T10:18:40Z

@susnux ah thank you for the explanation! :) I would just say let’s give it a bit more border-radius so that when it’s just one line it ends up being pill-style like all our buttons (and the current switcher style in Forms), as otherwise it will look out of place?

The radius should be 22px probably, or calc(var(--default-clickable-area) / 2). Then it should be nicely pill-style for single-line buttons and also work well for higher ones like in the sidebar.

susnux · 2023-04-03T11:43:35Z

@jancborchardt

I would just say let’s give it a bit more border-radius so that when it’s just one line it ends up being pill-style like all our buttons (and the current switcher style in Forms), as otherwise it will look out of place?

I changed that, it does now look like this:

raimund-schluessler · 2023-04-03T11:51:50Z

@jancborchardt

I would just say let’s give it a bit more border-radius so that when it’s just one line it ends up being pill-style like all our buttons (and the current switcher style in Forms), as otherwise it will look out of place?

I changed that, it does now look like this:

I think that is to much. calc(var(--default-clickable-area) / 2) i.e. 22px works better, I think. And the outer element needs a radius 2px larger than the inner one, to account for the border-thickness. Otherwise, there will be gaps at the corners.

src/components/NcCheckboxRadioSwitch/NcCheckboxRadioSwitch.vue

susnux · 2023-04-03T12:40:58Z

@jancborchardt @raimund-schluessler Do we want the same styling on the vertical group? Or do we want to keep the previous design for the vertical buttons?

I think having a consistent button style looks better, but what do you think?

both pills	keep previous

jancborchardt · 2023-04-03T12:46:14Z

@susnux totally agree with you, consistent style also for vertical buttons is best. :)

susnux · 2023-04-03T13:00:17Z

Ok I fixed the outer border, used the radius for both vertical and horizontal button groups and fixed the inner border margin. I think it is now ready to go.

It looks like this:

vokoscreenNG-2023-04-03_14-56-14.mp4

jancborchardt

Good stuff, let's go with that! :)

raimund-schluessler · 2023-04-03T20:54:51Z

@susnux works nicely now in the docs. I will test it in a real app tomorrow.
One of the unit tests still needs a fix, though.

skjnldsv · 2023-04-04T06:21:12Z

I find it a bit weird to have those pill rounded borders next to each others :)
I didn't thought that much into it, but couldn't we have straight borders when there is another button next to it and leave the rounded pill for the first and last items?

I'll let the design team take the decision, it's juste a thought :)

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

jancborchardt

Looks great now I think! :)

raimund-schluessler

Works well with the Tasks app. The failing test still needs a fix, but I have no idea, why it fails, honestly. Seems the NcCheckboxRadioSwitch does not emit the onToggle event in the tests, but it works well in a real app.

raimund-schluessler · 2023-04-04T07:42:26Z

tests/unit/components/NcAppSidebar/NcAppSidebarTabs.spec.js

 				expect(liList.length).toEqual(3)
 			})
 			it('emit "update:active" event with the selected tab id when clicking on a tab', () => {
-				const lastLink = wrapper.find('nav>ul>li:last-of-type>a')
+				const lastLink = wrapper.find('nav>.checkbox-radio-switch:last-of-type>label')
 				lastLink.trigger('click')


Maybe awaiting the trigger fixes the test?

Maybe awaiting the trigger fixes the test?

No, it does not 🙈

More info: a click on the <label> in <NcCheckboxRadioSwitch> triggers a click on <input>, but does not trigger a change event.

Seems to be either jsdom or VTU bug, but all related bug reports are fixed.

continues investigating

Signed-off-by: Grigorii Shartsev <grigorii.shartsev@nextcloud.com>

ShGKme · 2023-04-13T08:10:11Z

I added a fix for a test. Feel free to squash the commit.

More info (actually not much more): jsdom/jsdom#3143

https://github.com/jsdom/jsdom/blob/31cfdd4541e0cc83c2be9b105fda41bc188f72cd/lib/jsdom/living/nodes/HTMLInputElement-impl.js#L199-L202

ShGKme · 2023-04-13T11:36:28Z

I agree with @marcoambrosini about loosing the border.

In the previous design, there was no top and side borders, but a bottom border. It made the tab visually "a tab" and the content belove more "a tab content" related to the tabs.

With the new designed, it looks less tabs and more radio a switch to me. And adding a border around the button makes them even more a separated radio switch to me, not related to the content below.

How I "feel" it:

nimishavijay · 2023-04-13T12:43:23Z

Well, I believe that looking like radio buttons is kind of the idea! :) @ShGKme

Adding a border makes sure that:

The items are visually grouped together
It doesn't look too similar to a button, which is usually used to do some action. This component will be used to switch between multiple views
The alignment of the item is clear. Without the border, it is difficult to determine where the leftmost item begins, thereby making it look like the item is not aligned neatly with the text above or below it (for example, in Marco's video, it seems like "Search" is not neatly aligned with cat-picture.jpg / last edited 3 weeks ago, and adding a border would help)

ShGKme · 2023-04-13T13:27:24Z

Well, I believe that looking like radio buttons is kind of the idea!

But this element has a different semantics. Radio is for switching a value/selection while tabs are not only switching but also a navigation linked to panels. A layered sections. So the user should see that selected tab is also the selected panel.

What about adding only bottom border and remove border radius on the bottom (like web-browser tabs)?

susnux · 2023-04-17T10:44:56Z

Why I prefer the current style (borders and all round):

It is consistent with the other styles (we always have rounded elements
The border groups belonging elements
It makes clear where the element ends, so you know where to click (I think this is an important point regarding accessibility

raimund-schluessler · 2023-04-19T09:39:17Z

@jancborchardt I think we need a design decision here, and someone who feels confident to press "Merge" 😉

jancborchardt · 2023-04-19T14:15:28Z

I agree with the points brought up especially regarding accessibility and grouping.

For simplicity, I suggest:

We go with this design now and merge it, rounded and border around, as it is much better than what is there now and in line with the rest of our design
If we want to adjust its occurrence in the right sidebar navigation (as tabs) to use no border, we can very simply do that.

So I’ll merge it to unblock the situation. 🙇

susnux added 3. to review Waiting for reviews design Design, UX, interface and interaction design feature: app-sidebar Related to the app-sidebar component feature: checkbox-radio-switch Related to the checkbox-radio-switch component labels Apr 1, 2023

susnux requested review from jancborchardt, raimund-schluessler, ShGKme and nimishavijay April 1, 2023 01:14

susnux force-pushed the feat/nc-pill-menu branch from f75c5fd to 1e7ba44 Compare April 1, 2023 02:27

jancborchardt reviewed Apr 3, 2023

View reviewed changes

jancborchardt requested a review from marcoambrosini April 3, 2023 08:43

susnux force-pushed the feat/nc-pill-menu branch from 1e7ba44 to e6821e8 Compare April 3, 2023 11:42

raimund-schluessler reviewed Apr 3, 2023

View reviewed changes

src/components/NcCheckboxRadioSwitch/NcCheckboxRadioSwitch.vue Outdated Show resolved Hide resolved

jancborchardt reviewed Apr 3, 2023

View reviewed changes

src/components/NcCheckboxRadioSwitch/NcCheckboxRadioSwitch.vue Outdated Show resolved Hide resolved

raimund-schluessler reviewed Apr 3, 2023

View reviewed changes

src/components/NcCheckboxRadioSwitch/NcCheckboxRadioSwitch.vue Outdated Show resolved Hide resolved

susnux force-pushed the feat/nc-pill-menu branch from e6821e8 to bd14f63 Compare April 3, 2023 12:53

susnux requested review from jancborchardt and raimund-schluessler April 3, 2023 13:00

jancborchardt approved these changes Apr 3, 2023

View reviewed changes

raimund-schluessler requested review from skjnldsv and JuliaKirschenheuter April 3, 2023 20:46

susnux added 6 commits April 5, 2023 18:51

NcCheckboxRadioSwitch: Add icon slot for button variant

c586d3f

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

NcCheckboxRadioSwitch: Add border radius to button variant

45cd460

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

NcAppSidebarTabs: Use NcCheckboxRadioSwitch for the tabs navigation

724394d

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

tests: Fix tests for NcAppSidebarTabs

c7bc9c3

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

NcAppSidebar: Fix examples with invalid prop type

fe6fd54

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

NcCheckboxRadioSwitch: Add 2px group spacing

d932b34

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

susnux force-pushed the feat/nc-pill-menu branch from bd14f63 to d932b34 Compare April 5, 2023 16:52

jancborchardt approved these changes Apr 6, 2023

View reviewed changes

ChristophWurst mentioned this pull request Apr 6, 2023

Convert Email to Task nextcloud/mail#6127

Closed

2 tasks

raimund-schluessler approved these changes Apr 10, 2023

View reviewed changes

test(NcAppSidebarTabs): fix firing change event on radio in tests

a8fb1d8

Signed-off-by: Grigorii Shartsev <grigorii.shartsev@nextcloud.com>

hamza221 mentioned this pull request Apr 13, 2023

Create a task from a message nextcloud/mail#8375

Merged

3 tasks

jancborchardt merged commit bab5ba7 into master Apr 19, 2023

jancborchardt deleted the feat/nc-pill-menu branch April 19, 2023 14:15

raimund-schluessler added this to the 7.11.0 milestone Apr 24, 2023

artonge mentioned this pull request May 3, 2023

v7.11.0 #4029

Merged

skjnldsv added the enhancement New feature or request label May 5, 2023

nickvergessen mentioned this pull request May 5, 2023

[stable7] Revert new AppSidebarTabs design #4044

Merged

Antreesy mentioned this pull request May 8, 2023

Fix styles for long tab names in NcAppSidebarTabs #4064

Merged

st3iny mentioned this pull request May 10, 2023

Chore(deps): Bump @nextcloud/vue from 7.10.0 to 7.11.2 nextcloud/calendar#5201

Merged

ShGKme mentioned this pull request May 13, 2023

NcAppSidebar: fix tabs with css icon #4111

Merged

juliusknorr mentioned this pull request May 31, 2023

[BITV] 9.1.4.1/3.1 - Styling menubar - active contrast nextcloud/text#3860

Closed

1 task

ShGKme mentioned this pull request Nov 14, 2023

Fix tabs appearance #4815

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `NcCheckboxRadioSwitch` and use for `NcAppSidebarTabs` #3945

Improve `NcCheckboxRadioSwitch` and use for `NcAppSidebarTabs` #3945

susnux commented Apr 1, 2023 •

edited

Loading

jancborchardt left a comment

susnux commented Apr 3, 2023

jancborchardt commented Apr 3, 2023

susnux commented Apr 3, 2023

raimund-schluessler commented Apr 3, 2023

susnux commented Apr 3, 2023 •

edited

Loading

jancborchardt commented Apr 3, 2023

susnux commented Apr 3, 2023

jancborchardt left a comment

raimund-schluessler commented Apr 3, 2023

skjnldsv commented Apr 4, 2023 •

edited

Loading

jancborchardt left a comment

raimund-schluessler left a comment

raimund-schluessler Apr 4, 2023

raimund-schluessler Apr 10, 2023

ShGKme Apr 13, 2023

ShGKme commented Apr 13, 2023 •

edited

Loading

ShGKme commented Apr 13, 2023

nimishavijay commented Apr 13, 2023

ShGKme commented Apr 13, 2023

susnux commented Apr 17, 2023

raimund-schluessler commented Apr 19, 2023

jancborchardt commented Apr 19, 2023 •

edited

Loading

Improve NcCheckboxRadioSwitch and use for NcAppSidebarTabs #3945

Improve NcCheckboxRadioSwitch and use for NcAppSidebarTabs #3945

Conversation

susnux commented Apr 1, 2023 • edited Loading

Part 1: Improve NcCheckboxRadioSwitch styling

Before

With improvements (added borders)

With improvements

Part 2: Use NcCheckboxRadioSwitch for NcAppSidebarTabs

jancborchardt left a comment

Choose a reason for hiding this comment

susnux commented Apr 3, 2023

jancborchardt commented Apr 3, 2023

susnux commented Apr 3, 2023

raimund-schluessler commented Apr 3, 2023

susnux commented Apr 3, 2023 • edited Loading

jancborchardt commented Apr 3, 2023

susnux commented Apr 3, 2023

jancborchardt left a comment

Choose a reason for hiding this comment

raimund-schluessler commented Apr 3, 2023

skjnldsv commented Apr 4, 2023 • edited Loading

jancborchardt left a comment

Choose a reason for hiding this comment

raimund-schluessler left a comment

Choose a reason for hiding this comment

raimund-schluessler Apr 4, 2023

Choose a reason for hiding this comment

raimund-schluessler Apr 10, 2023

Choose a reason for hiding this comment

ShGKme Apr 13, 2023

Choose a reason for hiding this comment

ShGKme commented Apr 13, 2023 • edited Loading

ShGKme commented Apr 13, 2023

nimishavijay commented Apr 13, 2023

ShGKme commented Apr 13, 2023

susnux commented Apr 17, 2023

raimund-schluessler commented Apr 19, 2023

jancborchardt commented Apr 19, 2023 • edited Loading

Improve `NcCheckboxRadioSwitch` and use for `NcAppSidebarTabs` #3945

Improve `NcCheckboxRadioSwitch` and use for `NcAppSidebarTabs` #3945

susnux commented Apr 1, 2023 •

edited

Loading

Part 1: Improve `NcCheckboxRadioSwitch` styling

Part 2: Use `NcCheckboxRadioSwitch` for `NcAppSidebarTabs`

susnux commented Apr 3, 2023 •

edited

Loading

skjnldsv commented Apr 4, 2023 •

edited

Loading

ShGKme commented Apr 13, 2023 •

edited

Loading

jancborchardt commented Apr 19, 2023 •

edited

Loading