WIP Toolbar component and documentantion#781
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/dip-trasformazione-digitale/design-react-kit/H44trU3V3vdmj7QaNXubp5iNia8r |
|
Hello! I'd like to try to complete this task. I've opened a PR where I added basic component (in 3 sizes) and documentation. Please check if I'm doing right (this is the first time I doing that kind of stuff) and I will continue with it. |
dej611
left a comment
There was a problem hiding this comment.
🎉 Thanks for your contribution!
This is already a great start! I've left some comments about improvements that can be done on the current codebase.
Do you mind add also add a ToolbarDividerItem component?
src/Toolbar/ToolbarItem.tsx
Outdated
| url, | ||
| srText, | ||
| iconName = 'it-comment', | ||
| isIconSmall, |
There was a problem hiding this comment.
To be honest I feel this one should come from the Toolbar context rather than as a prop. I cannot find an instance of icon-sm outside the small version of the toolbar. But do not worry for now, will think about it later.
There was a problem hiding this comment.
Yes I feel so. If we add class 'toolbar-small' to Toolbar component, icons in ToolbarItem should be small too. That was my first intention but I couldn't find the way to do it for now.
There was a problem hiding this comment.
Don’t worry for now, it can be a follow up of this PR
There was a problem hiding this comment.
Meanwhile, can you remove it and set it to false internally where used?
There was a problem hiding this comment.
Removed, but now the ToolbarItem component lacks the 'small' size. In the EsempioSmall the icons are exactly the same size as they are in Esempio or EsempioMedium.
Thanks, that was my first commit to open source. :) I don't mind to add |
|
Checked the CI error and it’s just new snapshots not been added. You can do this on your machine and upload the new file: yarn test -u That should fix it |
dej611
left a comment
There was a problem hiding this comment.
The PR looks good. 🚀
If you can address only last few feedback I'd say it's ready to merge 👍
src/Toolbar/ToolbarItem.tsx
Outdated
| url, | ||
| srText, | ||
| iconName = 'it-comment', | ||
| isIconSmall, |
There was a problem hiding this comment.
Meanwhile, can you remove it and set it to false internally where used?
I would suggest adding it to the 'How to contribute' section as a necessary step |
dej611
left a comment
There was a problem hiding this comment.
All good! 🎉
Thanks for your contribution!
Fixes #
PR Checklist
masterbranch.Short description of what this resolves:
This is PR to add Toolbar component
Changes proposed in this Pull Request: