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

Modals: Mobile - Get alignment right #1599

Merged
merged 5 commits into from
Aug 2, 2023

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Aug 1, 2023

fix #1593

What this PR does

  • The informational modals have too much margin-top and margin-left and margin-right. This PR brings the text up by 36px so it's aligned with the X, and makes adjustments necessary for that.
  • The overall effect of getting all the margins right is that modals on mobile fit more text on the screen, and theoretically, are shorter now.

How to test this PR

  • Run thru the app in mobile and open every informational modal (2 login.gov, 1 contactless pay, 1 littlepay)

Docs: Informational Modals, specs

I wrote these out to help me get the alignment right.

Desktop

  • The height of the entire header section: 36px
image
  • Space between the edge and the begining of the text: 32px
image

Mobile

  • The distance between the edge and the top of the X/the begining of header text: 16px
image
  • Space between the edge and the begining of the text: 16px
image
  • I added the z-index declarations (and a margin-right) so that the X always comes on top of the header and the header never overlaps with the X. Now that the header is getting pushed up 36px into the header div, we need this z-index to ensure the clickability of the X.
image

@github-actions github-actions bot added the front-end HTML/CSS/JavaScript and Django templates label Aug 1, 2023
@machikoyasuda machikoyasuda added this to the Veterans milestone Aug 1, 2023
@machikoyasuda machikoyasuda self-assigned this Aug 1, 2023
@machikoyasuda machikoyasuda marked this pull request as ready for review August 1, 2023 01:14
@machikoyasuda machikoyasuda requested a review from a team as a code owner August 1, 2023 01:14
@machikoyasuda machikoyasuda linked an issue Aug 1, 2023 that may be closed by this pull request
Base automatically changed from fix/1575-modal-top to dev August 1, 2023 20:44
@machikoyasuda
Copy link
Member Author

Note: There's a change in the line-height of the modal titles on Mobile, which makes these 2-line titles tighter (vertically shorter) in Figma. But that's not part of this PR, that's part of #1573.

@thekaveman
Copy link
Member

thekaveman commented Aug 1, 2023

I think one of our paddings/margins needs to be adjust down to sm?

There is a funny break between 767px and 992px where the modal looks mobile (i.e. no padding at the top), but the content looks tablet/desktop.

750px

image

986px <--- this one

image

992px

image

Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

I think this makes the Close X on the Agency Selector modal a little too close to the text for certain sizes.

Sorry I though I did, but I hadn't updated my local branch with 587732b

Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

This is great!

Testing note: if your local site is already running, and you pull in new changes, some things will not rebuild

  • styles.css is fine if you hard refresh your browser, you'll get the updated version
  • most templates will be updated
  • I found in reviewing this PR that the selection-label--senior.html include, which is for the radio button on Eligibility Index, was not updating -- I had to restart my local site to get the change. This may be because it is referenced through a widget for a form field, which maybe gets compiled a single time at the beginning?

@machikoyasuda
Copy link
Member Author

@thekaveman Yea I noticed that when I work on some modals and the radio selector, I have to hard restart the whole site every time. I use the restart button on VS Code to make this faster/easier.

@machikoyasuda machikoyasuda merged commit 862ff63 into dev Aug 2, 2023
8 checks passed
@machikoyasuda machikoyasuda deleted the fix/1593-modal-mobile-margins branch August 2, 2023 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front-end HTML/CSS/JavaScript and Django templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mobile: Modal top margin is too long
2 participants