Skip to content
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

Allow minimum and maximum value for component number props #871

Merged
merged 11 commits into from
Sep 5, 2022

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Aug 30, 2022

Initially just intended for this PR to be prep for fixing this issue #826, by using a single source of truth for the minimum component dimensions. But it seems like it's actually a solution, so it closes #826.
Also closes #461

Using it already for Select component minimum width as an example.

Screen.Recording.2022-08-30.at.19.21.52.mov

@render
Copy link

render bot commented Aug 30, 2022

@apedroferreira apedroferreira self-assigned this Aug 30, 2022
@oliviertassinari oliviertassinari requested a deployment to prevent-component-overlap - toolpad-db PR #871 August 30, 2022 18:20 — with Render Abandoned
@Janpot
Copy link
Member

Janpot commented Aug 30, 2022

proptype is based on json-schema, I think that can describe min and max values. perhaps we should just reuse that?

@Janpot
Copy link
Member

Janpot commented Aug 31, 2022

I'm still not understanding the minimum width use case and how it should behave. Suppose minimum width is 50px, and I put 100 Selects in one row, what happens? what overflows or wraps?
Also, if a select can be the fullWidth of its container, and the container is resizable, then why does it need a width property?

@apedroferreira
Copy link
Member Author

proptype is based on json-schema, I think that can describe min and max values. perhaps we should just reuse that?

didn't know about that, i'll take a look

@apedroferreira
Copy link
Member Author

apedroferreira commented Aug 31, 2022

I'm still not understanding the minimum width use case and how it should behave. Suppose minimum width is 50px, and I put 100 Selects in one row, what happens? what overflows or wraps? Also, if a select can be the fullWidth of its container, and the container is resizable, then why does it need a width property?

i'm still not sure about all that so this PR was just going to add this feature which is probably helpful anyway. we can remove the width prop i added to the Select though for now.

i was planning to try things and see what might work or not, and if limiting resizing based on minimum width could be helpful or not, even if the UI can still overflow in some cases (there's probably no way we can really stop it from happening).

right now we do have that way of resizing width: full width + resizing containers, but i wasn't sure if we intend for that to be the main way to resize the width of components, and then add some margins controls on top. i guess that could work. maybe we could also add an ability to resize directly on the component at some point, as some users might expect, and having this width prop could help for those cases?

maybe we can just add this min/max value feature for now without adding a width prop to the Select, and then I can tackle the issue with components visually overlapping in some other way, maybe just by showing that things are overflowing but making it be less visually confusing.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 31, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 1, 2022
@apedroferreira
Copy link
Member Author

apedroferreira commented Sep 1, 2022

Looks like I already solved the whole issue in this PR after all, slightly unintentionally:

Screen.Recording.2022-09-01.at.16.40.43.mov

By not setting a minimum width directly in the component CSS this kind of issue does not happen, the components always adjust to the width of the container.

If we make the screen very very narrow and have a lot of components they do overflow as it's inevitable, but I think the best option is to just let that happen rather than hiding the overflow.

Screen Shot 2022-09-01 at 17 10 39

In the future we can probably improve support for small device screens.

So what do you think? In my opinion we can merge this.


If you don’t agree with adding the new width prop to Selects it’s also not strictly necessary, we can remove it and use whatever default values the MUI component uses.
In my opinion this width prop is useful without being detrimental - it gives the user more control over how the element looks, and it sets a minimum width that doesn’t look so bad when the select is empty, just like the minimum width before this PR.
But the width prop is just an extra thing that I can remove.

@apedroferreira
Copy link
Member Author

Also if you like this idea we can disable the width control when full-width is enabled, I just don't want to do it ahead of time if we don't want to go in this direction.

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

👌

@apedroferreira apedroferreira merged commit 56e579d into master Sep 5, 2022
@apedroferreira apedroferreira deleted the prevent-component-overlap branch September 5, 2022 15:18
@prakhargupta1
Copy link
Member

👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants