-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Upgrade react-day-picker
to v9
#4371
base: main
Are you sure you want to change the base?
Conversation
@AhmedBaset is attempting to deploy a commit to the shadcn-pro Team on Vercel. A member of the Team first needs to authorize it. |
react-day-picker
to v9react-day-picker
to v9
Thanks, @AhmedBaset, appreciate your work on this. However, I believe there's a bit more to address regarding Tailwind. In |
Thanks for the feedback. It's still WIP. |
@AhmedBaset Thanks for your work here. I've pinned react-day=picker to v8 for now. Let me know when this is ready for upgrade. (Ideally, if you get to it, it would be great to document the upgrade on the calendar page. See https://ui.shadcn.com/docs/components/input-otp#changelog as an example). |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Pinging @shadcn to review |
v9 adds new functionality which also breaks extra styling: |
Hello all! Please take a look at: I'm new to open-source, what's normally the way that people contribute? The code for above lives here. My approach is aware of the switch RDP made with the aria-selected attributes and seems to be exactly matching the styles of v8. I had hoped to write some UI regression tests with storybook but it wasn't that great at detecting the subtle visual changes. Currently having some teething issues with the navigation positioning when use opt to use a custom dropdown:
Code for the above can be found here. -Nick |
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 had to do the same
modifiers?.today && "bg-accent text-accent-foreground", | ||
modifiers?.selected && | ||
"bg-primary text-primary-foreground hover:bg-primary hover:text-primary-foreground focus:bg-primary focus:text-primary-foreground", | ||
modifiers?.outside && | ||
"text-muted-foreground opacity-50 pointer-events-none", | ||
modifiers?.disabled && "opacity-50 text-muted-foreground", | ||
modifiers?.hidden && "invisible", | ||
modifiers.range_middle && | ||
"bg-accent text-accent-foreground hover:bg-accent hover:text-accent-foreground rounded-none last:rounded-e-md first:rounded-s-md", | ||
modifiers.outside && | ||
modifiers.selected && | ||
"bg-accent/40 text-muted-foreground" |
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.
If this is going to be pursued, the code could be made a bit more readable with the ClassDictionary
from clsx
. Instead of having these &&
operators in every row, an object could be used like this:
modifiers?.today && "bg-accent text-accent-foreground", | |
modifiers?.selected && | |
"bg-primary text-primary-foreground hover:bg-primary hover:text-primary-foreground focus:bg-primary focus:text-primary-foreground", | |
modifiers?.outside && | |
"text-muted-foreground opacity-50 pointer-events-none", | |
modifiers?.disabled && "opacity-50 text-muted-foreground", | |
modifiers?.hidden && "invisible", | |
modifiers.range_middle && | |
"bg-accent text-accent-foreground hover:bg-accent hover:text-accent-foreground rounded-none last:rounded-e-md first:rounded-s-md", | |
modifiers.outside && | |
modifiers.selected && | |
"bg-accent/40 text-muted-foreground" | |
"bg-accent text-accent-foreground": modifiers?.today, | |
"bg-primary text-primary-foreground hover:bg-primary hover:text-primary-foreground focus:bg-primary focus:text-primary-foreground": modifiers?.selected, | |
"text-muted-foreground opacity-50 pointer-events-none": modifiers?.outside, | |
"opacity-50 text-muted-foreground": modifiers?.disabled, | |
invisible: modifiers?.hidden, | |
"bg-accent text-accent-foreground hover:bg-accent hover:text-accent-foreground rounded-none last:rounded-e-md first:rounded-s-md": modifiers?.range_middle, | |
"bg-accent/40 text-muted-foreground": modifiers?.outside && modifiers?.selected |
Also, I noted that some modifiers
values don't have the ?
. Don't know if it'd be a possible issue...
After looking into the daypicker types, I guess the ?
could be entirely removed for modifiers
?
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 came across this PR while updating day picker in my project.
An object like what @McTom234 suggested is definitely cleaner but the previous code (e.g. cell: ...
before it changed to day
) was targeting aria-selected
to add styling instead, maybe it would be better to keep that approach instead of importing Button
? Anyways that's what I'll be doing.
Also I think lines like "bg-primary text-primary-foreground hover:bg-primary hover:text-primary-foreground focus:bg-primary focus:text-primary-foreground": modifiers?.selected,
can be cleaned up since the default/hover/focus styles are the same, unless they're overriding styles set somewhere else.
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 agree on those lines, although the others are cleaner. I tried to upgrade to v9 myself, but didn't have the time to get a working solution done, so I stayed at v8 for now. But during the process, I felt like importing Button
wasn't a big gain.
Anyways, if you have a working solution example, I'd appreciate a little insight 😇
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.
@McTom234 for me, following the react-day-picker upgrading doc fixed most of the regressions. Seems like v9's HTML structure is different though so updating the nav buttons positioning isn't as straightforward. I'm working on that now but I'm using nav: "flex items-start",
to have the nav float up to the top, absolute left-2
and absolute right-2
on button_previous
and button_next
respectively, and months: "flex flex-col sm:flex-row", month: "m-4 mb-0 last-of-type:ml-0",
to adjust/fix some other layout regressions. Our calendar component is pretty heavily customized so I'm afraid that sharing it will be more noisy than helpful.
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.
Okay, thanks for your tips. I followed the same guide and had the issues with the nav buttons as well. That's where time-benefit wasn't given to proceed...
NICE! -> |
@shadcn Is there any ETA or planned time when this will be merged? We are waiting quite eagerly for this for a while now already. We mainly want to upgrade due to the |
"aria-selected:bg-accent aria-selected:text-accent-foreground", | ||
day_hidden: "invisible", | ||
month_caption: "flex justify-center items-center h-7", | ||
caption_label: "text-sm font-medium", |
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.
This breaks the styling when calendar captionLayout
is set to dropdown
.
I had success with
caption_label: "text-sm font-medium", | |
caption_label: | |
props.captionLayout !== "label" ? "hidden" : "text-sm font-medium", |
Close #4366
I'm opening this PR as early as possible to avoid the issues we had with
<Command />
whencmdk
v1.0 was released. And to make it easy for current users to upgrade in their codebases by copying the new update.This PR upgrades
react-day-picker
to the latest v9See Changelog and Upgrade Guide
Notable Changes:
date-fns
is now a dependency rather than a peer dependencyTODOs:
react-day-picker
to 9.0 to avoid the same issue with the next majorTo upgrade in your codebase:
pnpm update react-day-picker@9
pnpm un date-fns
If you don't use it anywhere elseshadcn-ui
release and runshadcn-ui add calendar --overwrite
or copy the source code directly for default or new york styles.