-
Notifications
You must be signed in to change notification settings - Fork 8
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
fix(ui-library): props alignment form caption #1023
Conversation
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.
You need to add the new 'hasIcon' prop to storybook. I would recommend doing it on line 46: https://github.com/deven-org/boiler/pull/1023/files#diff-4fd2a76c547f390341fc08e229facc46bf9c01c7d2cad98a90c769ef90971b74R46
Something like this:
hasIcon: { description: 'Choose if component has an icon.', defaultValue: true, control: { type: 'boolean', }, table: { category: 'Content / Settings', }, },
Then adding it again in const args =
on line 107:
https://github.com/deven-org/boiler/pull/1023/files#diff-4fd2a76c547f390341fc08e229facc46bf9c01c7d2cad98a90c769ef90971b74R107
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 would recommend renaming the content heading in form-caption index.stories.ts to 'Content / Settings' with an 's' to keep it inline with the other storybook files
c13d86a
to
d196d75
Compare
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.
This seems to work well for me. The caption icon will appear by default unless the user sets the icon to undefined. Unless we wanted to add an additional prop to each consuming component called 'showCaptionIcon'. Any thoughts @thrbnhrtmnn / @ChristianHoffmannS2 ?
@davidken91 I think I understand the issue now. If we add a "hasIcon" prop to the Form Caption, we would also have to add a similar prop to all components that consume the Form Caption, right? I am not sure if this is the best way to do this. My goal is just to align the behaviour how we show or hide icons for all components, if it makes sense. But maybe there are also reasons to not align this behaviour. Currently the Button Text and the Input Field Text component both have a prop to show/hide the icon and another prop to choose the icon. Other components like Icon, Button Icon and Select do not have such a prop. This makes sense as these components also always need to have an icon. I would suggest that we either add a hasIcon prop also to the FormCaption and all components that consume the Form Caption OR that we remove the hasIcon prop from Button Text and Input Field Text as well. We should do what is less effort. |
4cfb815
to
62fd41b
Compare
packages/ui-library/src/components/form-caption/index.stories.ts
Outdated
Show resolved
Hide resolved
62fd41b
to
232eb41
Compare
232eb41
to
1d94318
Compare
Co-authored-by: Thorben Hartmann <122102805+thrbnhrtmnn@users.noreply.github.com>
Co-authored-by: Thorben Hartmann <122102805+thrbnhrtmnn@users.noreply.github.com>
closes #1011