-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
prepareMeta function similar to prepareStory #20592
Conversation
code/ui/blocks/src/blocks/useOf.ts
Outdated
switch (resolved.type) { | ||
case 'component': { | ||
return { ...resolved, projectAnnotations: context.projectAnnotations }; | ||
} | ||
case 'meta': { | ||
return { | ||
...resolved, | ||
preparedMeta: prepareMeta(resolved.csfFile.meta, context.projectAnnotations), | ||
}; | ||
} | ||
case 'story': | ||
default: { | ||
return resolved; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one could argue that this functionality should be directly in DocsContext.resolveModuleExport
and not in useOf
.
I'm indifferent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can always change it later if we need it higher up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should split this a little more so it looks like:
function prepareStory(..) {
const partial = prepareAnnotations(...);
const unboundStoryFn = () => {....}
// ...
return {
...partial,
id: ..,
name: ...,
unboundStoryFn,
// ...
};
}
That way we aren't doing all the story function preparation for no reason with a mocked function.
code/lib/preview-api/src/modules/store/csf/prepareStory.test.ts
Outdated
Show resolved
Hide resolved
...preparedStory, | ||
// properties that are actually different between prepareMeta and prepareStory | ||
moduleExport: undefined, | ||
id: 'id--__meta', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't set an id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about that. componentAnnotations
have id
s, and all of the tests for prepareStory
pass in an id as the componentAnnotations
id.
I agree that we shouldn't set a fake one, we should just use the one from componentAnnotations
, I've changed that.
code/lib/preview-api/src/modules/store/csf/prepareStory.test.ts
Outdated
Show resolved
Hide resolved
code/lib/preview-api/src/modules/store/csf/prepareStory.test.ts
Outdated
Show resolved
Hide resolved
code/ui/blocks/src/blocks/useOf.ts
Outdated
switch (resolved.type) { | ||
case 'component': { | ||
return { ...resolved, projectAnnotations: context.projectAnnotations }; | ||
} | ||
case 'meta': { | ||
return { | ||
...resolved, | ||
preparedMeta: prepareMeta(resolved.csfFile.meta, context.projectAnnotations), | ||
}; | ||
} | ||
case 'story': | ||
default: { | ||
return resolved; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can always change it later if we need it higher up.
@tmeasday I refactored it so the story-specific stuff only happens in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, thanks @JReinhold
This PR implements a
prepareMeta
function, that similar toprepareStory
, combines project annotations with component annotations, but without story annotations.This is exposed in the
useOf
hook in blocks, that is then used by theDescription
block to render descriptions.This is just a refactor, no stories should change because of this.