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

fix: add whitespace-nowrap for Button, Select, Tab #266

Merged
merged 5 commits into from
Jan 12, 2024

Conversation

romanhrynevych
Copy link
Collaborator

This PR will add whitespace-nowrap class for Button, Select, Tab components, so there will be no overlapping when resizing, mentioned in #199

Also in this PR I refactored Button component Props to use VariantProps generic instead NonNullable<Parameters<typeof buttonVariants>>

@romanhrynevych romanhrynevych marked this pull request as draft January 11, 2024 21:06
@sadeghbarati
Copy link
Collaborator

I'm also working on refactoring components in this PR #241 which I handle button VariantProps in index.ts

Other changes are nice

@romanhrynevych
Copy link
Collaborator Author

I read about VariantProps in issue #199 , you mentioned if someone will make PR to refactor that, but if you already do that you can dismiss that commit 🙂

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Jan 11, 2024

The way you handle is ok, If you don't see the Vite overlay screen while you working on the Button in localhost


Individual Props types in defineProps macro ✅ https://github.com/joe-bell/cva/blob/main/examples/vue/src/components/Button.vue

Entire Prop type Object in defineProps macro ❌ not yet supported in Vue

@romanhrynevych
Copy link
Collaborator Author

@sadeghbarati need some advice from you, as I mentioned in this comment to make this we need to use tailwind custom grid-template-columns class grid-cols-[minmax(0,1fr)_auto], but I think that it will be not good to handle it like this in code, maybe I will add some custom tailwindcss extends props, what you think?

If thou, how it will be shown, don't know how to make it simple to reuse and understand, grid-cols-minmax-auto maybe?

image

@romanhrynevych
Copy link
Collaborator Author

romanhrynevych commented Jan 11, 2024

In original shadcn there is flex but with [&>span]:line-clamp-1, I think this is worse than truncate because we don't see ..., word just disappear

image

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Jan 11, 2024

That is a demo, user can change the styles of their component

But Yeah we can have truncate

Line clamp is also like truncate but in Y-axis

@romanhrynevych
Copy link
Collaborator Author

Yes, but what about custom TailwindCSS config grid cols, I can add them?

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Jan 11, 2024

Do you want to use it in Buttons Source (Registry Styles) or Demos?

@romanhrynevych
Copy link
Collaborator Author

I need to use it inside SelectTrigger.vue component, to limit selected span

image

@sadeghbarati
Copy link
Collaborator

I'm not a CSS Grid ninja but let's keep flex class cause width is auto by default in most elements

@romanhrynevych
Copy link
Collaborator Author

Ahaha, I will try to explain this) when we have fixed block with, for example, width of 180px Button, inside we have 2 elements (Text and Arrow), we know that Arrow is fixed width, but Text can be different width, so when we set display: flex; for Button, when Text will be wider, it will move our Arrow further, so we need to limit Text to all free space, so we add display: grid to Button and grid-templates-columns: minmax(0,1fr) auto;. Minmax make all magic and limit our Text width, and if we add truncate class it will add ... in the end of it. But with flex we cannot make same behaviour.

If you are assured that we can't use grid here, so we will make same as Original shadcn and limit by Y axis, but I think ... is more logical 🤔

P.S. About minmax magic I learned from official TailwindCSS docs, where they use them by default for grid-cols classes, but main problem that predefined classes divide our block to equal width columns, that will not work for us 🥲

@romanhrynevych
Copy link
Collaborator Author

Sorry for some misinformation, just googled some examples, and it can be fixed by min-width: 0 🤯
So we don't need grid for this, later will change everything, flex works fine 🙂

Ref: https://css-tricks.com/flexbox-truncated-text/

image

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Jan 11, 2024

https://twitter.com/t3dotgg/status/1745358867243171939

I'm with min-width : 0 xd


First person who tweeted this was Adam Wathan


Thanks for the time I'll check your PR next day
Hope you have a great day/night :cheers:

@romanhrynevych
Copy link
Collaborator Author

https://twitter.com/t3dotgg/status/1745358867243171939

I'm with min-width : 0 xd

Just saw this in my feed 5 mins ago 😂

@romanhrynevych
Copy link
Collaborator Author

@sadeghbarati PR is ready for review, added truncate to SelectTrigger, fixed whitespace-nowrap in Button, Tabs already has whitespace-nowrap

image

@romanhrynevych romanhrynevych marked this pull request as ready for review January 12, 2024 20:28
Copy link
Collaborator

@sadeghbarati sadeghbarati left a comment

Choose a reason for hiding this comment

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

Thanks 🫡

@sadeghbarati sadeghbarati merged commit 825f14e into radix-vue:dev Jan 12, 2024
1 check failed
@romanhrynevych romanhrynevych deleted the 199-bug-whitespace-nowrap branch January 12, 2024 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants