ToggleButton and ToggleButtonGroup #358

micgro42 · 2021-01-19T14:59:18Z

Works, but not sure if those props make any sense at all

github-actions · 2021-01-19T14:59:50Z

https://6ce012f7fc87adea939f393e60eec2b9033b4874--wmde-wikit.netlify.app

Ladsgroup · 2021-01-22T08:49:19Z

So far looks actually pretty okay and we can get it in IMHO, I don't see the point on calling this POC but the only thing is the you can't unselect a button (returning a group to its default state depending default state is selected a button or nothing is selected)

The story book also needs work (disabled state, etc.)

Ladsgroup · 2021-01-22T12:04:28Z

Everything I did is in this commit: 744d6c1

micgro42 · 2021-01-22T12:17:51Z

vue-components/src/components/ToggleButton.vue

+		background-color: $wikit-ToggleButton-disabled-background-color;
+	}
+
+	@media (max-width: $width-breakpoint-mobile) {


Looking at storybook, I have the strong suspicion that this media-query is not working

I double checked it before making the patch and it worked fine. Maybe you're checking against tablet breakpoint? The mobile breakpoint is pretty small.

My bad, the media-query is indeed working fine!
It is that box-sizing: border-box; is missing. That is why the buttons are so strangely large ^^

The m-sized buttons are supposed to have a total height of 32px, not 44px :)

Also, @SaiSan-WMDE , I think we might be missing some component tokens for the min-height.

Added box-sizing: border-box;

Hi! Thanks for working on this, and for adding box-sizing: border-box;. The size of buttons looks good now. We do have min-height tokens, but we haven't been using them with the button component because they proved to be unnecessary. Should that change?

Quick question: In they proved to be unnecessary, by they you mean the min-height css style or the min-height tokens?
The min-height css styles are in the figma specs and I set them as it's there. If we need to not use them, that's fine.

If that's about the tokens themselves, I'm fine with moving like this.

Using the CSS style, and therefore the tokens to define it, seemed to not have a real impact on the component, as far as I remember. I'd say we can move on like this.

I should be rephrasing many of the Figma specs, to avoid them from being interpreted as implementation decisions. Now that UX creates component tokens, these should be considered the design source of truth by developers.

(Leaving more feedback in a separate comment)

Done. Removed the min-height css.

vue-components/src/components/ToggleButtonGroup.vue

sai-san · 2021-01-25T10:32:38Z

This is looking fantastic. The main thing to solve from a UI point of view is still the border radius of the buttons that occupy a central position: for some reason is still not 0px.

Ladsgroup · 2021-01-25T11:03:05Z

This is looking fantastic. The main thing to solve from a UI point of view is still the border radius of the buttons that occupy a central position: for some reason is still not 0px.

I couldn't find any issues with on main toggle group but I saw the problem in the disabled one, It should be fixed now.

sai-san · 2021-01-25T11:36:43Z

I couldn't find any issues with on main toggle group but I saw the problem in the disabled one, It should be fixed now.

It appears that the buttons are not displaying the right border radius in none of the states (checking on Chrome/MacOS):

Buttons should look more integrated in the group. Adjacent borders should not display any roundness:

Ladsgroup · 2021-01-25T11:45:22Z

Mine looks different

Maybe it's an outdated version?

Ladsgroup · 2021-01-25T11:45:53Z

aha, it looks fine in firefox but not in chrome :(

Ladsgroup · 2021-01-25T11:48:56Z

aha, it looks fine in firefox but not in chrome :(

In Chrome it says "unknown property type" for border-start-start-radius: 0; and the rest.

Ladsgroup · 2021-01-25T11:52:44Z

It's not working on Chrome below 89 https://caniuse.com/?search=border-start-start-radius, we can have it now and slowly wait until it rolls out?

sai-san · 2021-01-25T11:57:13Z

aha, it looks fine in firefox but not in chrome :(

oh nooo. That's usually the other way around.

It's not working on Chrome below 89 https://caniuse.com/?search=border-start-start-radius, we can have it now and slowly wait until it rolls out?

I actually checked the component on Chrome 89 using BrowserStack and the borders still look broken :-/

sai-san · 2021-01-25T12:07:02Z

Should we use border-bottom-right-radius etc?

Ladsgroup · 2021-01-25T12:11:37Z

That would break in RTL languages :( We can go with it for now?

itamargiv · 2021-01-25T13:36:11Z

It's not working on Chrome below 89 https://caniuse.com/?search=border-start-start-radius, we can have it now and slowly wait until it rolls out?

The installed postcss plugin postcss-logical is supposed to take care of transpilation of logical properties into physical ones, and it is supposed to support border-start-end-radius and border-start-start-radius etc. We should check first if the problem is in our side or the implementation, and then check if there's a bug with the plugin.

Ladsgroup · 2021-01-25T14:29:21Z

This is very likely this: csstools/postcss-logical#27

Ladsgroup · 2021-01-25T15:25:43Z

This is very likely this: csstools/postcss-logical#27

Until that issue is resolved. I made a change that basically is the post-build of postcss and it should work just fine now.

itamargiv

Looking great, thanks for picking this up! I've added a request for change as a result of a small discussion above, I will review again after the design is approved.

vue-components/src/components/ToggleButtonGroup.vue

itamargiv · 2021-01-25T16:23:02Z

This is very likely this: csstools/postcss-logical#27

Until that issue is resolved. I made a change that basically is the post-build of postcss and it should work just fine now.

LOL, didn't see this comment, as I was on the review screen, sorry for repeating what you said.

sai-san · 2021-01-25T17:04:49Z

Agh, not sure what changed, but the borders were fixed before the last commit, and now they look off again 😮

Last thing from a design pov. The focus state needs fixing:

Would it be possible to apply the same logic and remove the border radius of the converging borders on focus? Ideally, they should look/behave like this:

It also looks like different focus styles are being applied to the buttons on the group? And that the tokens are being overridden somehow?

Ladsgroup · 2021-01-26T08:48:09Z

It's broken again because chrome doesn't :dir() either: https://caniuse.com/?search=dir()
Unbelievable

Ladsgroup · 2021-01-26T09:26:28Z

Also looking at output of postcss-logical, it outputs something like [dir="ltr"] .theselector which doesn't work if you don't set dir attribute or set it to "auto" (to get it from the language) which is the case for the storybook. I have a feeling that the storybook will look hideous in Persian.

itamargiv · 2021-01-26T13:24:42Z

Also looking at output of postcss-logical, it outputs something like [dir="ltr"] .theselector which doesn't work if you don't set dir attribute or set it to "auto" (to get it from the language) which is the case for the storybook. I have a feeling that the storybook will look hideous in Persian.

I think a quick way around it is to wrap the story with a div that sets the dir attribute.

itamargiv · 2021-01-26T13:59:28Z

It also looks like different focus styles are being applied to the buttons on the group? And that the tokens are being overridden somehow?

BTW, This can be fixed by setting outline: none on the :focus state of the child of the group and replacing it with an inset box-shadow.

sai-san · 2021-01-26T14:47:32Z

Ok, current status.

Firefox:

Border radius are fine 💯
Focus state is consistent, but there're a couple of issues:

One of the borders is always thinner than the other, but I understand this might not be easy to solve and therefore would make a compromise there.
There's an unwanted dotted border surrounding the buttons on focus

Chrome:

Same issues persist:

Inconsistent focus styles between buttons (and – same as Firefox – uneven widths)
Broken border radius (the border radius of central buttons should be 0px)

Ladsgroup · 2021-01-26T15:02:50Z

Are you sure you have the current version? This is the chrome for me now

Ladsgroup · 2021-01-26T15:12:52Z

Can't reproduce the dotted border in firefox either :(

vue-components/src/components/ToggleButton.vue

Co-authored-by: Sai_San <sarai.sanchez@wikimedia.de>

Ladsgroup · 2021-01-26T17:25:50Z

Sure. Applied.

sai-san · 2021-01-26T17:36:02Z

I was looking at the seemingly fresh Chromatic link. The one that technically was generated after adding outline:none and the dir property to the div (I checked the Files changed) 🤔
But anyway: the dotted line is finally gone after adding those extra outline:none's.

Full disclosure, the focus styles will be updated in a separate task that impacts tokens at a higher level (T272626), but that's a different story to be tackled by the constellation.

Here everything looks good! 💯 Thanks sooo much, Amir 🙏

* create skeleton * Update ToggleButton.json * Update tokens/properties/components/ToggleButton.json * Update ToggleButton.json * ToggleButton and ToggleButtonGroup (#358) * ToggleButton PoC * Second attempt: fix it in render * Silencing typescript errors * Use provide/inject * Fixed injection types * Add documentation to props and slots * Make ToggleButton ready for review * Fixes * Update vue-components/src/components/ToggleButton.vue Co-authored-by: Sai_San <sarai.sanchez@wikimedia.de> * Update vue-components/src/components/ToggleButton.vue Co-authored-by: Sai_San <sarai.sanchez@wikimedia.de> Co-authored-by: Amir Sarabadani <Ladsgroup@gmail.com> Co-authored-by: Sai_San <sarai.sanchez@wikimedia.de> * Final fixes Co-authored-by: Michael Große <michael.grosse@wikimedia.de> Co-authored-by: Amir Sarabadani <Ladsgroup@gmail.com>

ToggleButton PoC

354f3a1

micgro42 requested review from sai-san and itamargiv January 19, 2021 14:59

micgro42 added 2 commits January 19, 2021 17:26

Second attempt: fix it in render

598982a

Silencing typescript errors

3a81222

sai-san requested a review from a team January 20, 2021 10:24

micgro42 mentioned this pull request Jan 20, 2021

Toggle Proof of Concept #353

Closed

micgro42 added 3 commits January 20, 2021 15:13

Use provide/inject

12102a0

Fixed injection types

f74267a

Add documentation to props and slots

1205f30

Ladsgroup marked this pull request as ready for review January 22, 2021 11:54

micgro42 changed the title ~~ToggleButton PoC~~ ToggleButton and ToggleButtonGroup Jan 22, 2021

micgro42 commented Jan 22, 2021

View reviewed changes

Ladsgroup force-pushed the ToggleButtonPoC branch 4 times, most recently from 4dfb8f4 to ea7fc35 Compare January 25, 2021 10:19

Ladsgroup force-pushed the ToggleButtonPoC branch from ea7fc35 to 344d399 Compare January 25, 2021 11:02

Ladsgroup force-pushed the ToggleButtonPoC branch from 344d399 to a01fde1 Compare January 25, 2021 12:25

Ladsgroup force-pushed the ToggleButtonPoC branch from a01fde1 to 3e4c7e1 Compare January 25, 2021 15:24

itamargiv requested changes Jan 25, 2021

View reviewed changes

vue-components/src/components/ToggleButtonGroup.vue Outdated Show resolved Hide resolved

Ladsgroup force-pushed the ToggleButtonPoC branch from 3e4c7e1 to f999655 Compare January 25, 2021 16:25

Make ToggleButton ready for review

c58ff79

Ladsgroup force-pushed the ToggleButtonPoC branch from f999655 to c58ff79 Compare January 26, 2021 14:35

Fixes

544b1b2

sai-san reviewed Jan 26, 2021

View reviewed changes

vue-components/src/components/ToggleButton.vue Show resolved Hide resolved

sai-san reviewed Jan 26, 2021

View reviewed changes

vue-components/src/components/ToggleButton.vue Show resolved Hide resolved

Ladsgroup and others added 2 commits January 26, 2021 18:25

Update vue-components/src/components/ToggleButton.vue

97579c8

Co-authored-by: Sai_San <sarai.sanchez@wikimedia.de>

Update vue-components/src/components/ToggleButton.vue

deaf265

Co-authored-by: Sai_San <sarai.sanchez@wikimedia.de>

sai-san merged commit 5639a2b into create-toggle-button-tokens Jan 26, 2021

sai-san deleted the ToggleButtonPoC branch January 26, 2021 17:55

itamargiv mentioned this pull request Jan 27, 2021

Create toggle button component #350

Merged

ToggleButton and ToggleButtonGroup #358

ToggleButton and ToggleButtonGroup #358

Conversation

micgro42 commented Jan 19, 2021

github-actions bot commented Jan 19, 2021

Ladsgroup commented Jan 22, 2021

Ladsgroup commented Jan 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sai-san commented Jan 25, 2021

Ladsgroup commented Jan 25, 2021

sai-san commented Jan 25, 2021

Ladsgroup commented Jan 25, 2021

Ladsgroup commented Jan 25, 2021

Ladsgroup commented Jan 25, 2021

Ladsgroup commented Jan 25, 2021

sai-san commented Jan 25, 2021 • edited Loading

sai-san commented Jan 25, 2021

Ladsgroup commented Jan 25, 2021

itamargiv commented Jan 25, 2021

Ladsgroup commented Jan 25, 2021 • edited Loading

Ladsgroup commented Jan 25, 2021

itamargiv left a comment • edited Loading

Choose a reason for hiding this comment

itamargiv commented Jan 25, 2021

sai-san commented Jan 25, 2021 • edited Loading

Ladsgroup commented Jan 26, 2021

Ladsgroup commented Jan 26, 2021

itamargiv commented Jan 26, 2021

itamargiv commented Jan 26, 2021 • edited Loading

sai-san commented Jan 26, 2021 • edited Loading

Ladsgroup commented Jan 26, 2021

Ladsgroup commented Jan 26, 2021

Ladsgroup commented Jan 26, 2021

sai-san commented Jan 26, 2021 • edited Loading

sai-san commented Jan 25, 2021 •

edited

Loading

Ladsgroup commented Jan 25, 2021 •

edited

Loading

itamargiv left a comment •

edited

Loading

sai-san commented Jan 25, 2021 •

edited

Loading

itamargiv commented Jan 26, 2021 •

edited

Loading

sai-san commented Jan 26, 2021 •

edited

Loading

sai-san commented Jan 26, 2021 •

edited

Loading