Angular: Correctly set the 'required' argType #28718
Conversation
…quired: true}) is represented in compodoc output
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM
5 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 20a4456. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
| export const isRequired = (prop: Property): object => | ||
| prop.hasOwnProperty('required') && prop.required ? { required: true } : {}; | ||
|
|
There was a problem hiding this comment.
This seems a little misleading, with how it is named. Typically a is* represents a boolean. would it be better to change this to returning a boolean and changing the places this is used to required: isRequired(property). It doesn't change the end result, but I wonder if it would make the code more clear and the method more reusable. I don't have a strong opinion either way, it just stuck out as odd when I was reading the code and initially thought it was applying the spread operator to a boolean.
There was a problem hiding this comment.
Yeah good shout, I'll rework it a bit 👍
There was a problem hiding this comment.
One thing I want to be careful of though, re: returning a boolean, was most of the time it will return false (most @Inputs() don't have the required: true prop. Which is why i used the spread operator checking if the prop exists AND if it was set to true, and if not, return an empty object essentially not adding anything onto the argType.
It felt a bit redundant enhancing the argTypes with lots of required: false so prob want to avoid that if possible
There was a problem hiding this comment.
That makes sense. When I want to only add a property, I normally do something like ...(isRequired(property) ? { required: true } : {}). It is fairly easy to read, but I know it doesn't look great. If we want to use the method, like you had, a rename or additional method may work. This isn't a major piece of code that would be used outside of this file, so it doesn't matter much. An isRequired method that returns a boolean may not be needed, unless we need it for that other required property.
There was a problem hiding this comment.
I've updated that now, subtle difference but a bit clearer 😄
|
On the following line, should we consider the Also, is |
Yeah I have no idea about optional either 🤣 - happy to take a look though |
|
@Marklb - any idea when this would be ok to merge? Thanks |
| @@ -256,7 +263,7 @@ export const extractArgTypesFromData = (componentData: Class | Directive | Injec | |||
| category: section, | |||
| type: { | |||
| summary: isMethod(item) ? displaySignature(item) : item.type, | |||
| required: isMethod(item) ? false : !item.optional, | |||
| required: isMethod(item) ? false : item.required, | |||
There was a problem hiding this comment.
I was going to ask if this should be required: isMethod(item) ? false : isRequired(item), to avoid undefined, but I don't know if anything even uses this. I thought it used to do something, but I can't find where any of the other frameworks arg type extract functions seem to set it. Maybe if I dug some more I could find why it is here, but if the other frameworks aren't setting it then maybe it doesn't even need to be here.
There was a problem hiding this comment.
@Marklb You're right, i'm not sure it is even used. If i set that prop manually in a story, i.e:
argTypes: {
title: {
table: {
type: { required: true },
},
}
}
It does absolutely nothing.
However manually setting it this way:
argTypes: {
title: {
type: { name: 'string', required: true },
}
}
renders the correct asterix in red to say it's required (which was the intention of the PR)
That gets fixed by this change in the PR:
return { name: compodocType, ...setRequiredProperty(property) }; (line 153)
So i'm happy to either refactor to as you mentioned above, or remove entirely as part of the PR if we're confident it's no longer used - happy to take your steer on that :)
|
@valentinpalkovic Have you got a chance to look at this? I think it looks good. The only part I am undecided on is the |
|
Not yet. I am trying to plan it in for the next business days |
code/frameworks/angular/template/components/button.component.ts
Outdated
Show resolved
Hide resolved
valentinpalkovic
changed the title
| @@ -79,7 +79,7 @@ export const Object: Story = { | |||
| name: 'DEPRECATED: Object', | |||
| args: { | |||
| value: objectOptions.B, | |||
| argType: { options: objectOptions }, | |||
| argType: { options: objectOptions as any }, | |||
There was a problem hiding this comment.
This is technically an effect of a breaking change. We have to ensure that passing options the deprecated way is still supported and shouldn't trigger type issues for our users. We can definitely make the above type changes in Storybook 9, but not now. Can you adjust the PR to not throw away the custom types in code/lib/blocks/src/components/ArgsTable/types.ts, but leave a note instead to remove them in Storybook 9?
There was a problem hiding this comment.
@valentinpalkovic - i think you made this change to that file did you not? 🤔 9f440259ec06dc5c1faf336bb67b431ca10a041e
There was a problem hiding this comment.
Hehe. I knew the changes were kind of familiar to me xD. I'll take another look then.
There was a problem hiding this comment.
Hehe. I knew the changes were kind of familiar to me xD. I'll take another look then.
Heh no worries, I have weeks/months like that. Looked at so many code/projects I can't remember what I've done 😆
Closes #28706
What I did
Updated the
@storybook/angular package, where we are setting properties incompodoc.ts. Correctly set theargTypetype: { required: true}to berequired: trueif it is represented in the compodoc output for the component property incomponents.inputsClassproperty. This was not working when using compodoc and the property was there.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
button.component.tsso that the buttonlabelwas a required inputyarn task --task sandbox --start-from auto --template angular-cli/default-tslabelfield for thelabelcontrol onlyDocumentation
Did not add any specific documentation, as it is already implied by the docs that controls are inferred from the components themselves (via compodoc in this case) - this just fixes something that I already expected to work from the docs
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
This pull request corrects the
requiredargType for Angular components when using compodoc output.code/frameworks/angular/src/client/docs/compodoc.tsto correctly setrequiredattribute inargType.code/frameworks/angular/src/client/docs/compodoc.test.tsto cover required and non-required properties.requiredproperty toPropertyinterface incode/frameworks/angular/src/client/docs/types.ts.code/frameworks/angular/template/cli/button.component.tsandcode/frameworks/angular/template/components/button.component.tsto marklabelinput as required.