-
Notifications
You must be signed in to change notification settings - Fork 83
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
feat: restyle custom events modal #1062
Conversation
Fixed spacing and layout of event modal
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.
Comments on two aspects (only based on the SS provided):
- The day of week buttons being blue on an all-gray/all-white modal sticks out to me; could we try using a different variant (secondary should be gray?)
- The day of week buttons also get unaligned with the rest of the elements on wider modals. Can we style the buttons to take up 100% of horiz. space, by dynamically setting gap or width (I think width makes more sense)?
The UI tweaks look great -- just updating to say that I'll get around to reviewing this post finals 🫡 |
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.
Thanks for submitting your first PR! I've added a good handful of (mostly) UI-related tweaks and code quality suggestions.
I promise that I'm not always this pedantic, but it is good to learn (and embrace) our codebase standards early! Let me know if you have any questions 🫡
apps/antalmanac/src/components/Calendar/Toolbar/CustomEventDialog/CustomEventDialog.tsx
Outdated
Show resolved
Hide resolved
apps/antalmanac/src/components/Calendar/Toolbar/CustomEventDialog/CustomEventDialog.tsx
Outdated
Show resolved
Hide resolved
apps/antalmanac/src/components/Calendar/Toolbar/CustomEventDialog/CustomEventDialog.tsx
Outdated
Show resolved
Hide resolved
apps/antalmanac/src/components/Calendar/Toolbar/CustomEventDialog/CustomEventDialog.tsx
Outdated
Show resolved
Hide resolved
apps/antalmanac/src/components/Calendar/Toolbar/CustomEventDialog/DaySelector.tsx
Outdated
Show resolved
Hide resolved
apps/antalmanac/src/components/Calendar/Toolbar/CustomEventDialog/CustomEventDialog.tsx
Outdated
Show resolved
Hide resolved
apps/antalmanac/src/components/Calendar/Toolbar/CustomEventDialog/ScheduleSelector.tsx
Outdated
Show resolved
Hide resolved
apps/antalmanac/src/components/Calendar/Toolbar/CustomEventDialog/ScheduleSelector.tsx
Outdated
Show resolved
Hide resolved
apps/antalmanac/src/components/Calendar/Toolbar/CustomEventDialog/ScheduleSelector.tsx
Outdated
Show resolved
Hide resolved
apps/antalmanac/src/components/Calendar/Toolbar/CustomEventDialog/ScheduleSelector.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Kevin Wu <[email protected]>
I've committed the requested changes to the best of my knowledge. I'm still familiarizing myself with Typescript, so I totally forgot I had put "int" instead of the actual number type haha. Please let me know if any other edits need to be made, and sorry for the late response--I was out for most of winter break, so I'm just getting around to this now! |
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.
Nice work on the previous comments. Have a few more regarding the use of margin, but otherwise, it looks solid!
apps/antalmanac/src/components/Calendar/Toolbar/CustomEventDialog/CustomEventDialog.tsx
Outdated
Show resolved
Hide resolved
apps/antalmanac/src/components/Calendar/Toolbar/CustomEventDialog/CustomEventDialog.tsx
Outdated
Show resolved
Hide resolved
apps/antalmanac/src/components/Calendar/Toolbar/CustomEventDialog/DaySelector.tsx
Outdated
Show resolved
Hide resolved
apps/antalmanac/src/components/Calendar/Toolbar/CustomEventDialog/ScheduleSelector.tsx
Outdated
Show resolved
Hide resolved
Gotcha, I've replaced all margins now -- thanks for the advice regarding gap styling. |
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.
LGTM
Summary
Restyled the custom events modal:
It should look better with multiple schedules selected now.

Test Plan
Test on different device viewports: Computer (16x9); Tablet (iPad Pro); phone (iPhone 14 pro max); small phone (iPhone SE or any width < 400px)
Test with multiple schedules: 5, 10, 20 schedules selected & make sure the modal doesn't turn ugly.
Test that schedules are selected and the custom event is properly added for all schedules chosen
Test that schedules can be correctly deselected and that they don't have the custom event added to them
Test that the Building selector correctly searches up places in both the custom events modal and Map view, since I made a few changes to it
Issues
Closes #1025