Skip to content

Fix button toggles.#18860

Closed
jasmussen wants to merge 1 commit into
masterfrom
fix/toggle-state
Closed

Fix button toggles.#18860
jasmussen wants to merge 1 commit into
masterfrom
fix/toggle-state

Conversation

@jasmussen

Copy link
Copy Markdown
Contributor

Fixes #18825.

buttons

This restores the rules necessary for the delicate formatting. I know it's not pretty code, but I think we should restore it for now. We can revisit and very probably improve it a great deal if we look at the improvements from #18667. But that should be separate.

@jasmussen jasmussen added the [Type] Regression Related to a regression in the latest release label Dec 2, 2019
@jasmussen jasmussen self-assigned this Dec 2, 2019
@youknowriad

Copy link
Copy Markdown
Contributor

@jasmussen I thought we discussed this and agreed that consistency is better there :)

The problem here is that you'll notice that the dropdowns in the toolbar don't have the same styles. Also, this assumes that all toolbars are these small bordered rectangles which is not always the case, I don't think the default Toolbar component has these styles. For example, you can trigger the "Top Toolbar" mode and you'll see that these new styles are not great because they don't allign with the styles of the other buttons in the top toolbar.

I see two options:

  • Keep things as is in master
  • Target specifically the buttons of the block contextual toolbar and apply these overrides and avoid adding these in a generic way to all ToolbarButton components.

@gziolo

gziolo commented Dec 2, 2019

Copy link
Copy Markdown
Member

It regressed in the top toolbar as well, from master:
Screen Shot 2019-12-02 at 09 51 49

@youknowriad

Copy link
Copy Markdown
Contributor

Oh I guess, this restores the toggled state as well. So yeah! let's restore just that part (and not the hover styles etc...), also it makes me wonder why the toggled styles are not by default in the Button component instead of the Toolbar one. It seems that the component already has anisToggled prop so it should have the corresponding styles as well;

@gziolo

gziolo commented Dec 2, 2019

Copy link
Copy Markdown
Member

I opened PR which refactors code to stop using aria-pressed as a primary way to indicate that the button was pressed but promotes isToggled prop which was never styled:
#17748

Maybe we should land it first and apply all those styles using isToggled class name?

@gziolo

gziolo commented Dec 2, 2019

Copy link
Copy Markdown
Member

I also opened #18817 which ensures that we don't use button tag names in the toolbars directly, but rather we promote usage of Button or ToolbarButton components.

Putting those two together, it should help to make it much easier to style those buttons.

@gziolo

gziolo commented Dec 2, 2019

Copy link
Copy Markdown
Member

The problem here is that you'll notice that the dropdowns in the toolbar don't have the same styles.

Yes, I can see it:
Screen Shot 2019-12-02 at 10 09 23
Screen Shot 2019-12-02 at 10 09 28
Screen Shot 2019-12-02 at 10 09 41

@gziolo

gziolo commented Dec 2, 2019

Copy link
Copy Markdown
Member

Bonus thing, should it have any outline when focused even when it's disabled but focusable?

Screen Shot 2019-12-02 at 10 12 05

@jasmussen

Copy link
Copy Markdown
Contributor Author

The problem here is that you'll notice that the dropdowns in the toolbar don't have the same styles. Also, this assumes that all toolbars are these small bordered rectangles which is not always the case, I don't think the default Toolbar component has these styles. For example, you can trigger the "Top Toolbar" mode and you'll see that these new styles are not great because they don't allign with the styles of the other buttons in the top toolbar.

Yes, absolutely. But it's a somewhat non-trivial refactor, because the toggled state requires the hover style to be slightly inset.

I agree it's in need of a refactor, but it feels like this refactor might be better saved for exploring an implementation of the UI in #18667.

@jasmussen

Copy link
Copy Markdown
Contributor Author

To clarify a bit further, I don't have a strong visual attachment to how things look in master right now — consider this more of a "revert" PR for the toggle state. If @gziolo has a better refactor, definitely feel free to merge that instead of this one!

@gziolo

gziolo commented Dec 2, 2019

Copy link
Copy Markdown
Member

The problem here is that you'll notice that the dropdowns in the toolbar don't have the same styles. Also, this assumes that all toolbars are these small bordered rectangles which is not always the case, I don't think the default Toolbar component has these styles. For example, you can trigger the "Top Toolbar" mode and you'll see that these new styles are not great because they don't allign with the styles of the other buttons in the top toolbar.

Yes, absolutely. But it's a somewhat non-trivial refactor, because the toggled state requires the hover style to be slightly inset.

When I was playing with it on Friday, I wasted 2 or 3 hours, the issue was that the toggled button had this additional outline coming which aligned with the rest of buttons but it looked ugly 😅
I also tried to target with styles only those buttons which were toggled, can you try to do it? I failed :)

I agree it's in need of a refactor, but it feels like this refactor might be better saved for exploring an implementation of the UI in #18667.

I tend to agree. Let's apply the minimal set of changes to move forward.

@youknowriad

Copy link
Copy Markdown
Contributor

What about the minimal change for me to avoid regressions being a isToggled style added to the Button component.

@gziolo

gziolo commented Dec 2, 2019

Copy link
Copy Markdown
Member

I opened #18868 with an alternative fix proposed.

@jasmussen

Copy link
Copy Markdown
Contributor Author

Closing in favor of #18868.

@jasmussen jasmussen closed this Dec 2, 2019
@jasmussen jasmussen deleted the fix/toggle-state branch December 2, 2019 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Regression Related to a regression in the latest release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Blocks: Styles for the toggled state in the ToolbarButton removed

3 participants