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

Preview: Handle new docs-page index entries #18595

Merged

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Jun 29, 2022

Telescoping on #18574

Handle new docs entries types in the preview. In particular:

  • No longer allow rendering stories in docs mode
  • Refactor things a little for clarity (separate the two types of "docs render")
  • When rendering a "template" docs entry (attached to a story file), ensure we all the referenced CSF files (from storyImports also), to account for multiple files that define stories for the same component.

@nx-cloud
Copy link

nx-cloud bot commented Jun 29, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 79e5367. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@tmeasday tmeasday force-pushed the tom/sb-449-index-update-generation-to-produce branch 4 times, most recently from 379b85b to 75b818a Compare June 29, 2022 11:15
@tmeasday tmeasday force-pushed the tom/sb-451-preview-no-longer-allow-story-entries-to branch 2 times, most recently from 95919b8 to 05b184a Compare June 30, 2022 03:48
@tmeasday tmeasday requested a review from shilman June 30, 2022 03:50
@tmeasday tmeasday marked this pull request as ready for review June 30, 2022 03:50
@tmeasday tmeasday force-pushed the tom/sb-451-preview-no-longer-allow-story-entries-to branch from 3396d2b to ea331bf Compare July 1, 2022 02:06
@tmeasday tmeasday force-pushed the tom/sb-449-index-update-generation-to-produce branch from a682561 to 62153f3 Compare July 1, 2022 02:10
@tmeasday tmeasday force-pushed the tom/sb-451-preview-no-longer-allow-story-entries-to branch from 2d21f9b to 6cfdf34 Compare July 1, 2022 02:11
@tmeasday
Copy link
Member Author

tmeasday commented Jul 1, 2022

@shilman As you predicted the e2e-tests-sb-docs don't make any sense any more as they are testing each story in "docs" mode, which is no longer a real thing. We'll need to adjust the test runner to actually test docs entries I guess if we want to test docs in the runner.

What should I do with that for now? I'll disable the whole circle task, but if you think something different, let me know.

@tmeasday tmeasday force-pushed the tom/sb-449-index-update-generation-to-produce branch from 62153f3 to 70737e2 Compare July 6, 2022 09:12
@tmeasday tmeasday force-pushed the tom/sb-451-preview-no-longer-allow-story-entries-to branch from 68ea524 to d68956b Compare July 6, 2022 09:12
@ndelangen ndelangen marked this pull request as draft July 6, 2022 22:02
@tmeasday
Copy link
Member Author

tmeasday commented Jul 7, 2022

@ndelangen why did you make this a draft?

@tmeasday tmeasday marked this pull request as ready for review July 7, 2022 01:15
@tmeasday tmeasday force-pushed the tom/sb-449-index-update-generation-to-produce branch from 70737e2 to 095a5a9 Compare July 7, 2022 02:04
@tmeasday tmeasday force-pushed the tom/sb-451-preview-no-longer-allow-story-entries-to branch from 36dae59 to e19e16c Compare July 7, 2022 02:25
Base automatically changed from tom/sb-449-index-update-generation-to-produce to future/base July 7, 2022 23:39
@shilman shilman changed the title Handle new docs-page index entries in the preview Preview: Handle new docs-page index entries Jul 7, 2022
@@ -46,6 +47,16 @@ function focusInInput(event: Event) {
}

type MaybePromise<T> = Promise<T> | T;
type PossibleRender<TFramework extends AnyFramework> =
Copy link
Member

Choose a reason for hiding this comment

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

Why is this PossibleRender and not just Render? PossibleRender sounds like there may or may not actually be a render...

Copy link
Member Author

Choose a reason for hiding this comment

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

Render is the name of the interface. I could change that to BaseRender?

Copy link
Member

Choose a reason for hiding this comment

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

IRender? BaseRender sounds like it's providing some base functionality but it's just an interface. I wonder what's idiomatic TypeScript for this? I'm very familiar with this pattern from my OO days, but I haven't seen the analog to PossibleRender there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't really remember what it was like to code in an OO language but I think TS does stuff with type "elimination" that I don't recall from those days. Like if you have const x: A | B and then put if (x not of type A) in your code (that's pseudo code) TS will automatically cast it to B which is super handy.

Whereas if you have const x: I where I is an interface that A and B implement, it can't know that x isn't so other theoretical implementer, even if none exist.

@shilman shilman mentioned this pull request Jul 8, 2022
1 task
@shilman shilman merged commit 5d0c922 into future/base Jul 8, 2022
@shilman shilman deleted the tom/sb-451-preview-no-longer-allow-story-entries-to branch July 8, 2022 15:05
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.

2 participants