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.

Sign up for GitHub

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

Jump to bottom

Remove components-icon-button classname #19241

Merged
merged 2 commits into from
Dec 23, 2019
Merged

Remove components-icon-button classname #19241

merged 2 commits into from
Dec 23, 2019

Conversation

youknowriad
Copy link
Contributor

Follow-up to #19193

This PR removes the components-icon-button and uses a has-icon modifier instead per our guidelines.

Testing instructions

  • Make sure the components modified by this PR are still showing up properly with no styling regressions.

@youknowriad youknowriad self-assigned this Dec 19, 2019
@youknowriad youknowriad added [Feature] UI Components Impacts or related to the UI component system [Type] Code Quality Issues or PRs that relate to code quality Needs Dev Note Requires a developer note for a major WordPress release cycle labels Dec 19, 2019
@youknowriad youknowriad requested a review from aduth December 19, 2019 14:58
@gziolo
Copy link
Member

gziolo commented Dec 19, 2019

The only issue I see with this proposal is that we might want to support Button component which is text-based but has an additional icon on the left or right side. However, in that case we can always add another class name to make it more explicit 🤷‍♂

@aduth
Copy link
Member

aduth commented Dec 20, 2019

I've restarted the failing build a few times by now. I'm not sure if the current failures are legitimate or not. It's making it difficult to have confidence in approving this.

aduth
aduth reviewed Dec 20, 2019
View reviewed changes
packages/block-editor/src/components/block-switcher/style.scss
Comment on lines -6 to +7
.components-icon-button.block-editor-block-switcher__toggle,
.components-icon-button.block-editor-block-switcher__no-switcher-icon {
.components-button.block-editor-block-switcher__toggle,
.components-button.block-editor-block-switcher__no-switcher-icon {
Copy link
Member

Choose a reason for hiding this comment

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

Should I expect to see the .has-icon modifier class here?

I see there's lots of examples where we're not including the modifier class when removing the icon- portion. Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to avoid the extra specificity when not needed. In most of these examples the extra has-icon has no impact we just want more specificity over the button styles;

@youknowriad youknowriad force-pushed the remove/components-icon-button-classname branch from 7a828e6 to 888b20c Compare December 23, 2019 08:22
@youknowriad
Copy link
Contributor Author

The test failures were due to a misplaced publish button. It should be all good now.

@youknowriad youknowriad merged commit 3ca05a7 into master Dec 23, 2019
@youknowriad youknowriad deleted the remove/components-icon-button-classname branch December 23, 2019 08:52
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
@ockham ockham mentioned this pull request Jan 13, 2020
Merged
ockham added a commit to Automattic/jetpack that referenced this pull request Jan 13, 2020
@ockham
Gutenberg Blocks: Fix Jetpack sidebar button (#14327)
15d2b6b 
WordPress/gutenberg#19241 broke the styling of our Jetpack sidebar button  (again 😅) This is currently a problem on WP.com, where we just updated Gutenberg to 7.2.0.

This commit updates the class selectors accordingly.
jeherve pushed a commit to Automattic/jetpack that referenced this pull request Jan 16, 2020
@ockham @jeherve
[not verified] Gutenberg Blocks: Fix Jetpack sidebar button (#14327)
5c544e6 
WordPress/gutenberg#19241 broke the styling of our Jetpack sidebar button  (again 😅) This is currently a problem on WP.com, where we just updated Gutenberg to 7.2.0.

This commit updates the class selectors accordingly.
@jorgefilipecosta jorgefilipecosta mentioned this pull request Feb 12, 2020
Closed
23 tasks
@youknowriad youknowriad removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth left review comments

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@daniloercoli daniloercoli Awaiting requested review from daniloercoli

@ellatrix ellatrix Awaiting requested review from ellatrix

@etoledom etoledom Awaiting requested review from etoledom

@gziolo gziolo Awaiting requested review from gziolo

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@nerrad nerrad Awaiting requested review from nerrad

@ntwb ntwb Awaiting requested review from ntwb

@SergioEstevao SergioEstevao Awaiting requested review from SergioEstevao

@Soean Soean Awaiting requested review from Soean

@talldan talldan Awaiting requested review from talldan

Assignees

@youknowriad youknowriad

Labels
[Feature] UI Components Impacts or related to the UI component system [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
Gutenberg 7.2
Development

Successfully merging this pull request may close these issues.
3 participants
@youknowriad @gziolo @aduth