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: rename is-selected class #19820

Closed
wants to merge 1 commit into from
Closed

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Jan 22, 2020

Description

Blocks #19701.

This PR renames the block's is-selected class to the more descriptive is-appearing-selected class. The current class is misleading, as it may not be added even though the block is selected. The class is omitted when a block does not appear selected (when typing).

This change is necessary for blocks to be able to render their own wrapper as RichText also has a is-selected class, which causes a block to continuously look selected. The problem is not really the class duplication, but rather using is-selected to mean different things.

Some more complex block may have CSS rules using this class, but

  1. The class is anyway going to change meaning with the new G2 design (blocks will not have the selection border). It may even be removed since it won't be needed anymore.
  2. Classes are not an official API.
  3. When it is used, blocks are diverging from a consistent block styling.
  4. In some cases it seems to be combined with is-typing, which maybe indicates a need for a proper is-selected class. I consider adding a new class out of scope though.

How has this been tested?

Screenshots

Types of changes

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. .

@ellatrix ellatrix added the [Type] Code Quality Issues or PRs that relate to code quality label Jan 22, 2020
@ellatrix ellatrix requested review from youknowriad and aduth January 22, 2020 18:20
@youknowriad
Copy link
Contributor

i think we should hold on this PR because in G2 branch, is-selected means isSelected and there's no "selected style" anymore.

@ellatrix ellatrix requested a review from jasmussen January 22, 2020 18:22
@ellatrix
Copy link
Member Author

Sure, though it's blocking some other work. You could even remove the is-selected class in your branch? Is it still used for anything?

@youknowriad
Copy link
Contributor

@ellatrix I think some block still rely on it to show/hide things. what is this blocking?

@ellatrix
Copy link
Member Author

This:

This change is necessary for blocks to be able to render their own wrapper as RichText also has a is-selected class, which causes a block to continuously look selected. The problem is not really the class duplication, but rather using is-selected to mean different things.

It's fine if G2 is going to be merged this release cycle, but otherwise it would be good to fix in the meantime.

@youknowriad
Copy link
Contributor

youknowriad commented Jan 22, 2020

Why not change the is-selected used in RichText? seems less risky.

@ellatrix
Copy link
Member Author

Because it's misnamed by the block, not by rich text. The block doesn't apply the is-selected class consistently and it's actually using the class for showing the block as selected. That said, maybe is-selected classes should ideally be prefixed, e.g. is-rich-text-selected and is-block-selected. I'm fine with adjusting it for rich text.

@youknowriad
Copy link
Contributor

Because it's misnamed by the block, not by rich text

Right, it won't be misnamed though with G2. The problem here is that two components will impact the same DOM element and this is not that common. Not opposed to rename both but let's start with RichText cause I don't expect any third-party code to rely on this class.

@ellatrix
Copy link
Member Author

Agreed. Plus the class in rich text is only used for one thing (the placeholder).

@ellatrix ellatrix closed this Jan 22, 2020
@ellatrix ellatrix deleted the fix/block-is-selected-class branch January 22, 2020 18:49
@youknowriad
Copy link
Contributor

Wondering what does "is-selected" mean for RichText? Does it mean focused? can't we use :focus or something?

@ellatrix
Copy link
Member Author

I tried to use :focus but there seems to be a problem with selector specificity. I'll attempt to rewrite it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

2 participants