Skip to content
This repository has been archived by the owner on Sep 26, 2022. It is now read-only.

feat(FluidGrid): implement material-ui inspired grid #114

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

betaboon
Copy link

@betaboon betaboon commented Feb 2, 2021

first of thanks for this great package.

I added a grid that is inspired by reacts material-ui.

@vercel
Copy link

vercel bot commented Feb 2, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/thecomputerm/svelte-materialify/arhniw0fc
✅ Preview: https://svelte-materialify-git-fork-betaboon-feature-fluidgrid.thecomputerm.vercel.app

@Florian-Schoenherr
Copy link
Collaborator

(unofficial reviewer here, says code+feature is good)

@TheComputerM
Copy link
Owner

Can you specify the difference between FluidGrid and the other Grid system in the description?

@betaboon
Copy link
Author

betaboon commented Feb 2, 2021

Can you specify the difference between FluidGrid and the other Grid system in the description?

Yes. the documentation has to be fleshed out some more.
I wanted to get feedback on the feature as such before putting the time in for that first :)

/** sets alignment of grid items */
alignItems?: 'start' | 'center' | 'end' | 'stretch' | 'baseline';
/** sets column size at xs-breakpoint */
xs?: number | boolean;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure about the number | boolean types.
actually the number only accepts a certain range (1 through $grid-columns).
and the boolean only accepts true.
can that be expressed somehow? I'm new to typescript ;)


interface FluidGridProps {
/** whether this is a container */
container?: boolean;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at least one of container or item has to be set to true.
can that be expressed somehow ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use discriminated unions/algebraic data types when you want to declare mutually exclusive states.

interface FluidGridSharedProps {
  // all common props go here
}

interface FluidGridContainerProps extends FluidGridSharedProps {
  container: true;
  item?: false;
}

interface FluidGridItemProps extends FluidGridSharedProps {
  item: true;
  container?: false;
}

type FluidGridProps = FluidGridContainerProps | FluidGridItemProps

Be aware that as a union type rather than an interface there are more limitations placed on FluidGridProps so this approach has drawbacks to consider. One such drawback is that you cannot extend it with a new child interface:

interface MyCustomGridProps extends FluidGridProps {
  foo: string;
}

^^^ this will throw a compiler error

Instead you must declare a child as a type instead

type MyCustomGridProps = 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%;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure of height: 100% (and maybe align-items: center) has to go into FluidGrid.scss for stretch to work properly.

Copy link
Collaborator

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 :)

<Slider bind:value={values[label]}>{label}</Slider>
<Slider
bind:value={values[label]}
min={controls[label].min || 0}
Copy link
Author

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 of Slider which feels wrong as they might diverge.

Copy link
Collaborator

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? 😄

@betaboon
Copy link
Author

betaboon commented Feb 2, 2021

marked this PR as draft until I'm done :)

@betaboon
Copy link
Author

betaboon commented Feb 3, 2021

i just pushed a separate commit that removes the requirement of using global for the style.
I would very much like to get feedback on the approach chosen.

@nateshmbhat
Copy link
Contributor

any update on this PR ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants