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-docs: add escape hatch css classes in docpages #11775

Conversation

Jeremie-Chauvel
Copy link

@Jeremie-Chauvel Jeremie-Chauvel commented Aug 3, 2020

Issue: #8502

What I did

Implemented escape hatch to style docpages

How to test

  • Is this testable with Jest or Chromatic screenshots? yes -> update WIP
  • Does this need a new example in the kitchen sink apps? don't think so
  • Does this need an update to the documentation? probably WIP

If your answer is yes to any of these, please make sure to include it in your PR.

@Jeremie-Chauvel
Copy link
Author

I'm a little lost concerning where I should add the documentation of this change ?

Concerning the tests, I'm surprised that my change didn't break any ? Should I create an additionnal story with overriden style to test the change ? (In chromatic ?)

@Jeremie-Chauvel Jeremie-Chauvel marked this pull request as ready for review August 4, 2020 06:33
@ndelangen
Copy link
Member

@shilman This is something for us to consider...

I feel like we should -at some point- have a structured method for automatically adding these escape-hatch-css-classes to our components, rather then do it manually all over the place. We'll eventually forget them when we refactor.

I personally don't feel like adding a ton of tests for them either.
Maybe a few, to ensure the classes are there.

@stale
Copy link

stale bot commented Sep 7, 2020

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Sep 7, 2020
@stale stale bot removed the inactive label Sep 7, 2020
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Thanks for for fixing this @Jeremie-Chauvel!

Generally looks good. I'm wondering, though, in some cases you are passing className through various layers of indirection. Why can't we just set the className="sbdocs sbdocs-argstable" directly where its used rather than adding the extra complexity of passing it around as a variable?

@Jeremie-Chauvel
Copy link
Author

It's been long enough, I'm not quite confident I remember such decision reasons, I will investigate it on occasion.

@Jeremie-Chauvel Jeremie-Chauvel deleted the 8502-add-escape-hatch-css-classes-in-docpages branch December 21, 2020 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants