Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Components: Enforce consistent usage of Button and ToolbarGroup components #18817

Merged
merged 4 commits into from
Dec 3, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Nov 29, 2019

Description

It also extracts some changes from #17847, cc @diegohaz:

  • converts all button tag elements to Button components for consistency
  • replaces all usages of Toolbar components with ToolbarGroup in core blocks

In addition, this PR introduced ESLint change which discourages usage on button tag element in all production files. The only exception is save method implementation for the block definition.

How has this been tested?

npm run test-unit
npm run lint-js

Types of changes

Refactoring

Screenshots

Before

Screen Shot 2019-12-02 at 10 47 03
Screen Shot 2019-11-29 at 12 57 32
Screen Shot 2019-11-29 at 12 55 20
Screen Shot 2019-11-29 at 12 59 56

After

No visual changes 😃

Screen Shot 2019-12-02 at 12 17 43
Screen Shot 2019-12-02 at 10 44 58
Screen Shot 2019-11-29 at 12 55 31
Screen Shot 2019-11-29 at 12 55 54

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@gziolo gziolo added [Type] Code Quality Issues or PRs that relate to code quality [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Needs design efforts. labels Nov 29, 2019
@gziolo
Copy link
Member Author

gziolo commented Nov 29, 2019

There are two issues with CSS specificity which I couldn't resolve myself:

Tabs in the sidebar still need a fix for the outline when focused:
Screen Shot 2019-11-29 at 12 54 46

Inserter item still needs a fix for the background color when focused and hovered:
Screen Shot 2019-11-29 at 12 54 59

I would appreciate some help.

@gziolo gziolo changed the title Components: Enforce consisten usage of Button component Components: Enforce consistent usage of Button and ToolbarGroup components Nov 29, 2019
@gziolo gziolo mentioned this pull request Dec 2, 2019
@jasmussen
Copy link
Contributor

Pushed a fix for the two things you mentioned.

@gziolo
Copy link
Member Author

gziolo commented Dec 2, 2019

I can confirm that the inserter item was fixed. The focused tab item which isn't the selected one still looks different than in master:

This branch:
Screen Shot 2019-12-02 at 10 46 27

master:
Screen Shot 2019-12-02 at 10 47 03

the outline should be dotted.

@gziolo gziolo self-assigned this Dec 2, 2019
@gziolo gziolo added General Interface Parts of the UI which don't fall neatly under other labels. and removed Needs Design Needs design efforts. labels Dec 2, 2019
@jasmussen
Copy link
Contributor

Fixed 👍 👍

@gziolo
Copy link
Member Author

gziolo commented Dec 2, 2019

Thanks @jasmussen, everything looks good now. You are my CSS superhero 🥇

@gziolo gziolo force-pushed the update/button-cross-platform branch from e31bff8 to e1f908c Compare December 2, 2019 14:57
@gziolo
Copy link
Member Author

gziolo commented Dec 2, 2019

After rebasing with the `master I noticed one unexpected change in the UI:

Screen Shot 2019-12-02 at 16 00 59

@jasmussen
Copy link
Contributor

jasmussen commented Dec 3, 2019

I see this:

html

The focus rectangle is missing from the toggled state, and it could use a border radius like the others. Let me know if you need help with that.

@gziolo
Copy link
Member Author

gziolo commented Dec 3, 2019

This is what I see in this branch:
html-block

Should we update the hover state?

@gziolo gziolo force-pushed the update/button-cross-platform branch from e1f908c to ae766c7 Compare December 3, 2019 08:49
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a solid step forward, and one that will enable further enhancements through code simplification. The HTML/Preview isn't perfect, but that "toolbar tab" pattern deserves some thought separately. As such and from a design POV, this looks good to me.

@gziolo gziolo merged commit c0ba214 into master Dec 3, 2019
@gziolo gziolo deleted the update/button-cross-platform branch December 3, 2019 09:23
@youknowriad
Copy link
Contributor

Nice work here

@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). General Interface Parts of the UI which don't fall neatly under other labels. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants