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

feat: restyle custom events modal #1062

Merged
merged 6 commits into from
Jan 10, 2025
Merged

Conversation

xgraceyan
Copy link
Contributor

Summary

Restyled the custom events modal:

  • Changed schedule select from checkbox to chip multiple select
    • This is so not all the schedules appear in the main modal (the mass of checkboxes for every schedule is what I'm assuming is mainly contributing to the "ugliness" of the modal)
  • Added select variant to Building select props to maintain consistency of outlined style
    • Building select in custom events modal is now outlined variant, while in Maps it is the filled variant
image

It should look better with multiple schedules selected now.
image

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

Copy link
Member

@KevinWu098 KevinWu098 left a 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):

  1. 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?)
  2. 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)?

@xgraceyan
Copy link
Contributor Author

Gotcha! I've made changes according to your comments:

  • As far as I've checked, the secondary theme is white, not gray; I've made the buttons be the default color theme instead, which is gray. However, I think maybe the selector would look better if the buttons selected were the primary blue color, instead of also being gray, as it feels slightly bland right now -- what do you think?
image
  • I've also fixed the alignment of the day select buttons now. They should resize on larger modals and take up all of the horizontal space.
image

@xgraceyan xgraceyan requested a review from KevinWu098 December 4, 2024 04:38
@KevinWu098
Copy link
Member

The UI tweaks look great -- just updating to say that I'll get around to reviewing this post finals 🫡

Copy link
Member

@KevinWu098 KevinWu098 left a 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 🫡

@xgraceyan
Copy link
Contributor Author

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!

@xgraceyan xgraceyan requested a review from KevinWu098 January 6, 2025 07:10
Copy link
Member

@KevinWu098 KevinWu098 left a 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!

@xgraceyan
Copy link
Contributor Author

Gotcha, I've replaced all margins now -- thanks for the advice regarding gap styling.

@xgraceyan xgraceyan requested a review from KevinWu098 January 10, 2025 01:20
@KevinWu098 KevinWu098 changed the title Restyle custom events modal feat: restyle custom events modal Jan 10, 2025
Copy link
Member

@KevinWu098 KevinWu098 left a comment

Choose a reason for hiding this comment

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

LGTM

@KevinWu098 KevinWu098 merged commit c9ae1af into main Jan 10, 2025
4 checks passed
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.

Restyle Custom Events Modal
2 participants