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

Block Editor: Update BEM syntax to CSS modifer guidelines #19738

Merged
merged 2 commits into from
Feb 7, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented Jan 17, 2020

Previously: #17846 (comment), #16790 (comment)

This pull request seeks to update incorrect usage of BEM modifier syntax to use the is--based modifier syntax as recommended in the CSS naming guidelines.

This impacts two components:

  • LinkControl
  • ResponveBlockControl

In the latter case, the implementation has been revised as mentioned at #16790 (comment) to unify the application of a single is-responsive modifier class.

Open Question: There was a hidden attribute included previously in ResponsiveBlockControl which would never be applied given how the logic flowed. The changes here seek only to preserve the current behavior. If it needs to be applied, I can restore it as appropriate.

Testing Instructions:

Repeat testing instructions from #17846 and #16790, verifying that there are no regressions.

Ensure tests pass:

npm test

@@ -91,9 +91,9 @@ describe( 'Basic rendering', () => {

const toggleState = container.querySelector( 'input[type="checkbox"]' ).checked;

const defaultControlGroup = container.querySelector( '.block-editor-responsive-block-control__group--default' );
const defaultControlGroup = container.querySelector( '.block-editor-responsive-block-control__group:not(.is-responsive)' );
Copy link
Member

Choose a reason for hiding this comment

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

It's a bummer that this requires the :not pseudo-selector. I suppose it's not particularly an issue here, but selectors that might potentially have several modifiers will need extra care; scoping a selector for a component's "default" state could require multiple :not checks that could get difficult to manage. Hopefully that's a relatively rare case though.

Copy link
Member

Choose a reason for hiding this comment

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

(And I suppose it's more likely to be the case in tests like this; it's probably possible to architect without it being a concern in the CSS itself, and especially if some of the Emotion explorations move forward.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I see this as mostly a use-case we'd want to isolate within tests, where we're explicitly wanting to distinguish between the default and the variant.

In real-world application, we should typically want to treat the variant as something of an additive layer upon the default, even to the extent of unsetting/overriding properties from those defaults, vs. ever feeling the need to use :not( .is-variant ) in CSS. If we found ourselves needing to use :not, it probably raises a bigger question of whether we can really consider there to be a default (vs. other named variants, or treating them as wholly different elements).

@aduth aduth merged commit 4e3a617 into master Feb 7, 2020
@aduth aduth deleted the update/link-control-bem branch February 7, 2020 22:52
@github-actions github-actions bot added this to the Gutenberg 7.5 milestone Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants