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

Doc Blocks: Fix styling and parameter bugs #20803

Merged
merged 18 commits into from
Feb 1, 2023
Merged

Doc Blocks: Fix styling and parameter bugs #20803

merged 18 commits into from
Feb 1, 2023

Conversation

JReinhold
Copy link
Contributor

@JReinhold JReinhold commented Jan 26, 2023

Closes #20850

What I did

This PR fixes a few outstanding bugs with docs and blocks.

  1. Canvases in autodocs didn't respect parameters.docs.canvas.withToolbar = false - they do now.
  2. We introduced a regression that broke Canvas when used in unattached mode and without any story children (deprecated API). This has now been fixed and has an internal story. This was reported here [Bug]: Unable to use Description without a Story in MDX #19964 (comment)
  3. inheritable styles like fonts were set on the docs content root, which would bleed through Unstyled blocks. We no longer set that reset, and it was a mistake it got set in the first place. This ensures that Unstyled removes styles from fonts.
  4. Headers were not unstyled when wrapped in Unstyled. They are now. This happened because we still replace headers in MDX with custom components, and they had local styling. Now that local styling is removed so they rely solely on the global styles, which Unstyled can disable.

There's still a difference in header styles between autodocs and manual docs, specifically I've noticed that h2 in autodocs have font-weight 400, while they have font-weight 700 in manual docs. font-weight 400 is a global style we set at the manager level via Emotion, and I can't figure out why that global style is not set in manual docs. It's not because it's overwritten, it is never there.
If anyone has inputs to this let me know.

How to test

  • See new internal story for Canvas in UI SB
  • See the Unstyled docs page in UI SB
  • See new stories for Stories and Primary blocks in UI SB

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"]

@JReinhold JReinhold added the bug label Jan 26, 2023
@JReinhold JReinhold self-assigned this Jan 26, 2023
@renato-bohler
Copy link

renato-bohler commented Jan 27, 2023

Would that fix #20780?

code/ui/blocks/src/blocks/DocsStory.tsx Outdated Show resolved Hide resolved
@tmeasday
Copy link
Member

@JReinhold I see font-weight 700 on h2s in autodocs?

image

@JReinhold
Copy link
Contributor Author

@JReinhold I see font-weight 700 on h2s in autodocs?

image

Uh... I don't? Neither on next nor in this branch. This is from the published SB in this branch.
What's going on? It's even the same class hash.

image

Co-authored-by: Tom Coleman <[email protected]>
@JReinhold
Copy link
Contributor Author

Would that fix #20780?

Yes, but that was also fixed by #20780 so no need to here: 👍

@JReinhold
Copy link
Contributor Author

@tmeasday I've added a commit that fixes #20745, and reacted to your feedback, could you re-review?

in essence I added initalCode to the SourceContext, that will always be the first snippet rendered. when Canvas gets forceInitial it will make sure to only use initialCode and not code. This breaks down if any of our frameworks for some reason emits different sources initially, as we'd "stop" at the first one. But I don't know if that is a scenario that exists.

(I've changed story.__forceInitialArgs prop on the Canvas only to __forceInitial as it now affects both the args and the code)

If we wanted a more proper solution for this we should probably stop rendering the same story twice in autodocs and instead generate a virtual story that we'd reference to ensure they don't collide. But I don't think now is the time for that.

@JReinhold JReinhold requested a review from tmeasday January 27, 2023 22:34
@tmeasday
Copy link
Member

tmeasday commented Jan 27, 2023

@JReinhold the rest of the changes look good, but I don't think 16eb696 is the right fix here.

It would be an issue if you

a) changed the args
b) visited the docs page

Then the primary story would render & emit with the updated args, and the second render would use + emit the story's initialArgs. Which one would reach the source container first... I'm not sure it's defined, but probably the primary :)

Maybe pull that commit into a second PR and let's discuss further there?

The solution suggested by @shilman was to somehow add an extra description prop to the Story + Source blocks to tie them together (that prop's value would get passed up to the SourceContainer somehow). So we'd render like:

<Story of={...} sourceLabel="primary" />
<Source of={...} sourceLabel="primary" />

WDYT about that? (don't love the name sourceLabel, and not sure if it should be internal or not).

@JReinhold
Copy link
Contributor Author

@JReinhold the rest of the changes look good, but I don't think 16eb696 is the right fix here.

It would be an issue if you

a) changed the args b) visited the docs page

You're right, I missed that.

Maybe pull that commit into a second PR and let's discuss further there?

Deal!

The solution suggested by @shilman was to somehow add an extra description prop to the Story + Source blocks to tie them together (that prop's value would get passed up to the SourceContainer somehow). So we'd render like:

<Story of={...} sourceLabel="primary" />
<Source of={...} sourceLabel="primary" />

WDYT about that? (don't love the name sourceLabel, and not sure if it should be internal or not).

So sort of like having a "sub story id" to key and get sources by, I think that makes sense. I don't know if it should be hardcoded by the rendering components, or if we should just generate a uuid to use instead.

@tmeasday
Copy link
Member

or if we should just generate a uuid to use instead.

Are you imagining each Canvas would generate its own uuid? I guess that could work, but would have to be stable across re-renders.

I'm also wondering if (one day) we might also allow more than one set of arg values for the same story. We could use a similar "sub story id" concept to disambiguate the events. So the Controls block would need to know about the sub story id also.

@JReinhold
Copy link
Contributor Author

JReinhold commented Feb 1, 2023

@tmeasday I changed all stories for blocks to be decorated by the DocsContainer - using parameters.docsStyles = true. I kept seeing some inconsistencies between docs view and story view for the blocks, and hopefully this should make it more consistent, as they now inherit the same global styles as in docs view.

This does add some unnecessary margin to all the stories, but I think that's a fine tradeoff. pixels are free.

(Chromatic snapshots are acting up here, have reported)

@JReinhold JReinhold merged commit dd1ca7f into next Feb 1, 2023
@JReinhold JReinhold deleted the jeppe/fix-blocks branch February 1, 2023 22: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.

Fix Doc Block docs, bugs and migration notes
3 participants