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
Improvement/cal 639 turn edit location dialog into radix uu #1055
Improvement/cal 639 turn edit location dialog into radix uu #1055
Conversation
alishaz-polymath
commented
Oct 27, 2021
•
edited
edited
- Removes headless UI from event-types/[type].tsx
- Replaces Modal with Dialog in Edit Location
- Adds react-hook-form
- Fixes Radio button bug
CAL-639 turn “edit location” dialog into radix-uu
if you find a minute, looks like this dialog is still legacy Reported by Peer. |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/cal/calendso/CtaVNAqhGcDvymvXszyU6BszobYV |
42735a9
to
dd30bcd
Compare
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.
Font size on period types is a bit too big, minor thing:
I've managed to reproduce the bug with multiple locations. It happens when you open the dialog, then cancel, then drop the select again, cancel.. rince repeat and you'll break it more and more. Up to you if you want to take a look or have a look (or someone else) in a dedicated PR.
Code itself is a big improvement, well done
pages/event-types/[type].tsx
Outdated
|
||
const [hidden, setHidden] = useState<boolean>(eventType.hidden); | ||
console.log("periodType", periodType, eventType.periodType); |
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.
Small debug line left
Yeah, that bug is frustrating. I haven't been able to pinpoint the reason behind it. It's in prod, and I couldn't figure out why this is, hence not really worked on it in this PR. |
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.
Thank you for doing this! I left some comments about minor details.
value={asStringOrUndefined(eventType.schedulingType)} | ||
options={schedulingTypeOptions} | ||
onChange={(val) => { | ||
formMethods.setValue("schedulingType", val); |
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.
Also here:
Argument of type 'ChangeEvent<HTMLSelectElement>' is not assignable to parameter of type 'SchedulingType'.
Type 'ChangeEvent<HTMLSelectElement>' is not assignable to type '"COLLECTIVE"'.
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 one is actually related to the components/ui/form/radio-area/RadioAreaGroup
definition of onChange. I'm not sure what's the best way to proceed in this case.
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 think we can make this in a follow up PR. It's good enough for now 👍🏽
30d3a34
to
ce6c61d
Compare