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

Made the calendar page mobile-responsive #450

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

devlarabar
Copy link
Contributor

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!

Screenshot 2023-08-28 134434
Screenshot 2023-08-28 134530
Screenshot 2023-08-28 134604
Screenshot 2023-08-28 134622
Screenshot 2023-08-28 134643

Type of change

Please select everything applicable. Please, do not delete any lines.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • This change requires an update to testing

Issue

Checklist:

  • This PR is up to date with the main branch, and merge conflicts have been resolved
  • I have executed npm run test and all tests have passed successfully or I have included details within my PR on the failure.
  • I have executed npm run lint and resolved any outstanding errors. Most issues can be solved by executing npm run format
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings

@devlarabar devlarabar temporarily deployed to DEV August 28, 2023 22:42 — with GitHub Actions Inactive
Copy link
Member

@Caleb-Cohen Caleb-Cohen left a 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.

@cblanken
Copy link
Contributor

cblanken commented Sep 9, 2023

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.
image
image

We probably want to make it fullscreen once we get to smaller form factors like this
image

Looking at the code though it looks like it gets kinda messy. We'll need to rework the styles in UserForm.js, EventModal.jsx, CalendarPage.js and Backdrop.jsx so I don't know if we'd want to include that in this PR as part of the calendar page or just make a separate issue for the modals.

@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"
Copy link
Contributor

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?

Copy link
Member

@Caleb-Cohen Caleb-Cohen Sep 10, 2023

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?

I think tailwind config is a better way to go in the long run. @devlarabar what are your thoughts?

Copy link
Contributor Author

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!

Copy link
Member

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.

Copy link
Contributor

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.

@Caleb-Cohen
Copy link
Member

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. image image

We probably want to make it fullscreen once we get to smaller form factors like this image

Looking at the code though it looks like it gets kinda messy. We'll need to rework the styles in UserForm.js, EventModal.jsx, CalendarPage.js and Backdrop.jsx so I don't know if we'd want to include that in this PR as part of the calendar page or just make a separate issue for the modals.

@Caleb-Cohen thoughts?

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.

@cblanken
Copy link
Contributor

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.

@Caleb-Cohen
Copy link
Member

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?

@cblanken
Copy link
Contributor

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"
Copy link
Contributor

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.

@cblanken cblanken merged commit 5285afa into Together-100Devs:main Oct 16, 2023
1 check 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.

None yet

3 participants