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

Addon Outline: Fix additional outline border in docs mode #21773

Merged
merged 5 commits into from May 9, 2023

Conversation

g-cappai
Copy link
Contributor

@g-cappai g-cappai commented Mar 25, 2023

Closes #21328

What I did

I changed the selector used to target elements to be outlined.
Now every descendant of the direct child of the ZoomElement component is outlined according to its type.

How to test

See issue -> to reproduce.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@g-cappai g-cappai changed the title Change outline selector in docs Fix: additional outline border in docs mode Mar 25, 2023
@ndelangen ndelangen added the bug label Mar 27, 2023
Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

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

Hey @g-cappai thanks a lot for your contribution! I think we should make some slight changes to make this more reliable.

What about we add a data-story-block="true" to the Story block, and then we target that selector instead?

@g-cappai
Copy link
Contributor Author

g-cappai commented Apr 25, 2023

Hi @yannbf , I totally agree. Now it looks definitely better.

@g-cappai g-cappai requested a review from yannbf May 2, 2023 19:06
Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

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

This looks great, thank you so much for working through the feedback!

cc @JReinhold in case you want to take a look at this

@yannbf yannbf added the patch:yes Bugfix & documentation PR that need to be picked to main branch label May 5, 2023
@yannbf yannbf changed the title Fix: additional outline border in docs mode Addon Outline: Fix additional outline border in docs mode May 9, 2023
@yannbf yannbf merged commit 7fc998c into storybookjs:next May 9, 2023
54 checks passed
@JReinhold
Copy link
Contributor

@yannbf I'm sorry I'm late to the party here, but given that the story block already has a unique class "sb-story", couldn't we have used that as a selector? Looks like it would effectively do the same thing as what we do with the data attribute.

@yannbf
Copy link
Member

yannbf commented May 10, 2023

@yannbf I'm sorry I'm late to the party here, but given that the story block already has a unique class "sb-story", couldn't we have used that as a selector? Looks like it would effectively do the same thing as what we do with the data attribute.

In my perspective the data attribute is safer because the classname could be changed in the future and the person might not realize its connection to the addon outline, that's why I suggested it! Also feels more descriptive than a sb-story class

@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label May 12, 2023
shilman pushed a commit that referenced this pull request May 12, 2023
Addon Outline: Fix additional outline border in docs mode
@shilman shilman mentioned this pull request May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: outline bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: An additional outline border is displayed in docs mode
5 participants