-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat(SLB-306): Info Grid #226
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.
@elistone There are just two comments which indicate probable bugs. Indicated with 🐛
The rest are nit picks 🙂
// Your new icon is now available for selection, it will be displayed everywhere all icons are used. | ||
// Or you can use the limitedIconListOption function to limit the available icons in a specific context. | ||
// | ||
// NOTE: search this file for "*" to find the places where you need to add your new icon. |
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 simplify it. I think we can use TS types here in the way that no comments will be needed.
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.
Actually I'm not sure if this file is needed at all. Because everything it contains is used only once, and maybe can be inlined (?)
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.
Well my idea here is that it should be used more that once, anywhere we give options to select icons in blocks.
I already can see in the CTA buttons we give the option for arrow icons before or after text but nothing gets displayed to the user while making these changes.
Also icon selection is quite a common request for PMs, so I was trying to make a reusable and dynamic libarary just for use on the Gutenberg side.
icons: Icons[], | ||
addDefault: boolean = true, | ||
defaultLabel: string = 'Select an icon', | ||
defaultValue: string = '', |
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.
At the moment the function is used with only the first arg passed. Everything else is always default.
It looks like the function is designed with future use-cases in mind. But that future might never come 😛
Description of changes
Adds info grid block.
How has this been tested?