-
Notifications
You must be signed in to change notification settings - Fork 247
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
Make previewChips use the grid system as well #173
Conversation
Hi @anthonyraymond , Thanks for your contribution, I'd ask a code review to @max-carroll because he recently worked on the |
Hi @panz3r I will have a look at this either tonight or first thing tomorrow |
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.
Looks good, however needs the latest changes pulled in (which I'm sorry which will require a small bit of extra work since I deprecated a few of the things you used)
Actually @panz3r I just noticed this is atop of master and not v4.x so this should be fine, however if this is to be merged into v4.x my changes will need to be considered, what do you think? |
I suppose the other alternative is we cherry pick the change into v4.x and fix it where it's not as desired |
I suppose that since we are going to consider the changes you made to the Preview section a breaking change (thus the The idea being to leave behind a |
In that case I will change my review to "looks good" |
approve changes... sorry I have been institutionalised by TFS, if you ever see TFS anywhere, avoid it 🤣 |
Hi @anthonyraymond , Thanks for your contribution and for your patience (also thanks to @max-carroll for reviewing the code 👍 ). The PR looks good, as @max-carroll suggested could you please add a little example inside
|
Before this PR `previewGridProps` and `previewGridClasses` had no effect if we switched to `useChipsForPreview` and all the chips got stacked one below the other because of the use of a simple div. Using the already well established **previewGrid** props allow for much more chip customizations. Chips are now displayed inline by default but thanks to the `previewGridProps` this can be changed at any time
Hi guys, thanks for your review. I've added an example, but honestly i don't know if such a small thing deserves a documentation, before that PR the previewChip system wasn't aware of the component properties, i've just used the already existing properties. |
Description
Before this PR
previewGridProps
andpreviewGridClasses
had no effect if we switched touseChipsForPreview
and all the chips got stacked one below the other because of the use of a simple div.Using the already well established previewGrid props allow for much more chip customizations.
Chips are now displayed inline by default but thanks to the
previewGridProps
this can be changed at any timeType of change
How Has This Been Tested
I've tested it personally in one of my projects
Test Configuration:
Checklist