Skip to content

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

Merged
gziolo merged 4 commits into
masterfrom
update/button-cross-platform
Dec 3, 2019
Merged

Components: Enforce consistent usage of Button and ToolbarGroup components#18817
gziolo merged 4 commits into
masterfrom
update/button-cross-platform

Conversation

@gziolo

@gziolo gziolo commented Nov 29, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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

gziolo commented Nov 29, 2019

Copy link
Copy Markdown
Member Author

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
Closed
@jasmussen

jasmussen commented Dec 2, 2019

Copy link
Copy Markdown
Contributor

Pushed a fix for the two things you mentioned.

@gziolo

gziolo commented Dec 2, 2019

Copy link
Copy Markdown
Member Author

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

jasmussen commented Dec 2, 2019

Copy link
Copy Markdown
Contributor

Fixed 👍 👍

@gziolo

gziolo commented Dec 2, 2019

Copy link
Copy Markdown
Member Author

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

gziolo commented Dec 2, 2019

Copy link
Copy Markdown
Member Author

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

Screen Shot 2019-12-02 at 16 00 59

@jasmussen

jasmussen commented Dec 3, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

gziolo commented Dec 3, 2019

Copy link
Copy Markdown
Member Author

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
jasmussen
jasmussen approved these changes Dec 3, 2019
View reviewed changes

@jasmussen jasmussen left a comment

Copy link
Copy Markdown
Contributor

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

youknowriad commented Dec 3, 2019

Copy link
Copy Markdown
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

Reviewers

@jasmussen jasmussen jasmussen approved these changes
@ajitbohra ajitbohra Awaiting requested review from ajitbohra
@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten
@ellatrix ellatrix Awaiting requested review from ellatrix
@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta
@noisysocks noisysocks Awaiting requested review from noisysocks
@Soean Soean Awaiting requested review from Soean
@talldan talldan Awaiting requested review from talldan
@youknowriad youknowriad Awaiting requested review from youknowriad
@enriquesanchez enriquesanchez Awaiting requested review from enriquesanchez
@mapk mapk Awaiting requested review from mapk

Assignees

@gziolo gziolo

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

Milestone

Gutenberg 7.1

Development

Successfully merging this pull request may close these issues.

3 participants

@gziolo @jasmussen @youknowriad