- Notifications
You must be signed in to change notification settings - Fork83
feat(FluidGrid): implement material-ui inspired grid#114
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
vercelbot commentedFeb 2, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect:https://vercel.com/thecomputerm/svelte-materialify/arhniw0fc |
Florian-Schoenherr commentedFeb 2, 2021
(unofficial reviewer here, says code+feature is good) |
TheComputerM commentedFeb 2, 2021
Can you specify the difference between FluidGrid and the other Grid system in the description? |
betaboon commentedFeb 2, 2021
Yes. the documentation has to be fleshed out some more. |
| /** sets alignment of grid items */ | ||
| alignItems?:'start'|'center'|'end'|'stretch'|'baseline'; | ||
| /** sets column size at xs-breakpoint */ | ||
| xs?:number|boolean; |
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'm unsure about thenumber | boolean types.
actually thenumber only accepts a certain range (1 through$grid-columns).
and theboolean only acceptstrue.
can that be expressed somehow? I'm new to typescript ;)
| interfaceFluidGridProps{ | ||
| /** whether this is a container */ | ||
| container?:boolean; |
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 least one ofcontainer oritem has to be set to true.
can that be expressed somehow ?
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 can usediscriminated unions/algebraic data types when you want to declare mutually exclusive states.
interfaceFluidGridSharedProps{// all common props go here}interfaceFluidGridContainerPropsextendsFluidGridSharedProps{container:true;item?:false;}interfaceFluidGridItemPropsextendsFluidGridSharedProps{item:true;container?:false;}typeFluidGridProps=FluidGridContainerProps|FluidGridItemProps
Be aware that as a union type rather than an interface there are more limitations placed onFluidGridProps so this approach has drawbacks to consider. One such drawback is that you cannot extend it with a new child interface:
interfaceMyCustomGridPropsextendsFluidGridProps{foo:string;}
^^^ this will throw a compiler error
Instead you must declare a child as a type instead
typeMyCustomGridProps=FluidGridProps&{foo:string}
^^^ this will compile
I haven't reviewed much of the code base to check if this approach is being used for this project but hopefully this answers your question.
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| height: 100%; |
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'm unsure ofheight: 100% (and maybealign-items: center) has to go intoFluidGrid.scss forstretch to work properly.
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 don't see any difference without it, also it would be an easy change if someone has problems with it :)
| <Sliderbind:value={values[label]}>{label}</Slider> | ||
| <Slider | ||
| bind:value={values[label]} | ||
| min={controls[label].min||0} |
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.
these newly added settings duplicate the defaults ofSlider which feels wrong as they might diverge.
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.
works for now, or what would you do instead? 😄
Uh oh!
There was an error while loading.Please reload this page.
betaboon commentedFeb 2, 2021
marked this PR as draft until I'm done :) |
betaboon commentedFeb 3, 2021
i just pushed a separate commit that removes the requirement of using |
nateshmbhat commentedApr 1, 2021
any update on this PR ? |
first of thanks for this great package.
I added a grid that is inspired by reacts material-ui.