-
Notifications
You must be signed in to change notification settings - Fork 111
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
Made the calendar page mobile-responsive #450
Conversation
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! Thanks for taking this on.
This looks great. I like the more compact header for mobile. One thing I did notice is that the EventModal popup is pretty cramped when in mobile dimensions. We probably want to make it fullscreen once we get to smaller form factors like this Looking at the code though it looks like it gets kinda messy. We'll need to rework the styles in @Caleb-Cohen thoughts? |
@@ -11,7 +11,7 @@ const MonthAndYear = ({ | |||
<div className="flex"> | |||
<button onClick={handlePreviousMonth}> | |||
<svg | |||
className="w-6 h-6" | |||
className="w-6 h-6 max-[440px]:w-4 max-[440px]:h-4" |
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 haven't played around with the breakpoints myself at this point, but do we think these 440px
and 380px
breakpoints should be refactored into the Tailwind config for easier reuse (or if they need to be updated later) or are they more of a one-off kind of thing?
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 haven't played around with the breakpoints myself at this point, but do we think these
440px
and380px
breakpoints should be refactored into the Tailwind config for easier reuse (or if they need to be updated later) or are they more of a one-off kind of thing?
I think tailwind config is a better way to go in the long run. @devlarabar what are your thoughts?
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.
Hey, sorry I've only just now seen this! I agree that the tailwind config is the better way to go, in case there are more cases like this in the future if other features are added to the app. I'd be happy to work on the mobile-responsiveness of the modal as well if needed, either in this PR or a different one!
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'd prefer not to add it to this PR because there are a couple ways we could solve the modal that'd I'd like to investigate before we start working on it.
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 should be good then, so I'll go ahead and approve as well. I'll make sure to include some language in the tech spec I'll be writing later this week for the modal/mobile design and specifically for the tailwind breakpoints.
While I agree it could really benefit from some style love, I think this is out of scope for what @devlarabar originally signed up for and what I approved. |
Fair enough. I'll write up a separate issue to address the mobile-responsive styling for the modals. |
Agreed. Will you be at standup on Tuesday to discuss this as well? |
Yep I'll be there 👍 |
@@ -11,7 +11,7 @@ const MonthAndYear = ({ | |||
<div className="flex"> | |||
<button onClick={handlePreviousMonth}> | |||
<svg | |||
className="w-6 h-6" | |||
className="w-6 h-6 max-[440px]:w-4 max-[440px]:h-4" |
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 should be good then, so I'll go ahead and approve as well. I'll make sure to include some language in the tech spec I'll be writing later this week for the modal/mobile design and specifically for the tailwind breakpoints.
Description
The calendar page is now mobile-responsive; the issue before this change was that the calendar header was too wide, and was not resizing properly when the screen size changed. Using only Tailwind CSS, this has been fixed. Please see the screenshots below for examples of what the header now looks like at different breakpoints!
Type of change
Please select everything applicable. Please, do not delete any lines.
Issue
Checklist:
npm run test
and all tests have passed successfully or I have included details within my PR on the failure.npm run lint
and resolved any outstanding errors. Most issues can be solved by executingnpm run format