add_named_slot_for_icon_to_design_system_components_with_label #386
Conversation
bereket-WMDE
force-pushed
the
add_named_slot_for_icon_to_design_system_components_with_label
branch
from
15be122
to
1ba92f2
Compare
| @@ -9,10 +9,13 @@ | |||
| :checked="checked" | |||
| :disabled="disabled" | |||
| > | |||
| <slot v-if="this.textDirection == 'rtl'" name="suffix" /> | |||
There was a problem hiding this comment.
No. We don't want to explicitly specify with props whether this is ltr or rtl. There are HTML5 technologies that do this for us. The easiest might be flexbox.
Have a look at this commit: 667cb5b
There was a problem hiding this comment.
looks much cleaner
|
Finally leaving some comments here after some discussion and trying some things out. I already had a chat with Bereket about all this, but:
|
- implemented flexbox to auto detect text direction
As far as i know, the Icon has to be part of the label, in order to to achieve the desired result. but we don't want that, since the Icon has a different purpose (opening the popover component). right now, clicking the label would check the checkbox. But that could be my limited styling knowledge :) I am happy to hear ideas from other devs. |
Fixed in the new commit. |
| @if $displayType == block { | ||
|
|
||
| @if $displayType == block or $displayType == inline-flex { |
There was a problem hiding this comment.
Why do we need this change?
There was a problem hiding this comment.
we don't. I forgot to remove that from a previous commit.
removed now
| &__label { | ||
| @include Label( block ); | ||
| display: inine; |
There was a problem hiding this comment.
typo: this should probably be inline? Similarly in the other files.
However, this line can just be removed, it is obviously not needed as it is currently ignored and the every looks fine.
There was a problem hiding this comment.
ah.. ofc this line is useless. thanks
|
Not blocking this PR, but I'm now confused by the token that we are using. Why are we using |
Co-authored-by: Sai_San <sarai.sanchez@wikimedia.de>
TODO: - Tests - (Maybe in another PR) once wmde/wikit#386 merged and release, use it to add tooltip to components Bug: T275542
…omponents_with_label
|
Approving this PR so we can move forward, although at some point (probably while working on this Phab ticket? Though the change does not only apply to checkboxes) we should come up with a way to align the icon with the top or the baseline of the first line of wrapping labels, instead of centering it in relation to the label text. This would prevent misalignment like the following:
I'll document the fix in the linked ticket. |
Yes, i agree with the approach |
bereket-WMDE
deleted the
add_named_slot_for_icon_to_design_system_components_with_label
branch
Bug: T274346