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

VACMS-15967: Fix search bar spacing in Common Tasks Section #1818

Merged
merged 7 commits into from
Dec 12, 2023

Conversation

chriskim2311
Copy link
Contributor

@chriskim2311 chriskim2311 commented Dec 2, 2023

Summary

Change the margin property to a larger screen size media query so there is enough left hand space for the search bar at certain screen widths

Related issue(s)

Testing done

Tested across multiple screen lengths to check for common tasks section spacing issues.

Screenshots

Screen width 1010px.
VA_gov_Home___Veterans_Affairs

What areas of the site does it impact?

Homepage

Acceptance criteria

…ent-build into 15967-homepage-common-tasks-css-fix
@chriskim2311 chriskim2311 marked this pull request as ready for review December 4, 2023 18:09
@chriskim2311 chriskim2311 requested review from a team as code owners December 4, 2023 18:09
@va-vfs-bot va-vfs-bot temporarily deployed to master/main/15967-homepage-common-tasks-css-fix December 4, 2023 18:52 Inactive
Copy link
Contributor

@randimays randimays left a comment

Choose a reason for hiding this comment

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

We need some adjustments around ~768-923px wide, the Top pages section is still too close to the right edge of the screen

Screenshot 2023-12-04 at 1 54 06 PM

@va-vfs-bot va-vfs-bot temporarily deployed to master/main/15967-homepage-common-tasks-css-fix December 4, 2023 20:57 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to master/main/15967-homepage-common-tasks-css-fix December 5, 2023 18:27 Inactive
Copy link
Contributor

@randimays randimays left a comment

Choose a reason for hiding this comment

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

Can we shrink the right padding on Top pages down to 16px? The links are wrapping just a bit too much.

Screenshot 2023-12-05 at 2 13 16 PM

@chriskim2311 chriskim2311 requested a review from a team as a code owner December 5, 2023 23:47
@va-vfs-bot va-vfs-bot temporarily deployed to master/main/15967-homepage-common-tasks-css-fix December 6, 2023 00:09 Inactive
@randimays
Copy link
Contributor

Apologies, I should have worded that a bit more clearly; I was only referring to the padding on the right side of the page (for Top pages). I think we need the wider padding on the left side of Top pages. Did you get a consult with Jordan yet?

Copy link
Contributor

@maxx1128 maxx1128 left a comment

Choose a reason for hiding this comment

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

Should we have equal spacing on the left sides of both "Search" and "Top Pages" for when both sections are squished? This is the page at 865px wide, and the left side spacing being different for both is throwing me off.

Screen Shot 2023-12-06 at 11 12 44 AM

@chriskim2311
Copy link
Contributor Author

@thejordanwood Hi! Can you take a look at this PR env and let me know what you think would be best case forward. Randi and Max have made good points. Let me know what you recommend and I can implement the changes.

http://39ffe3b77fc34ce9f5758fa9dd4c0d1e.review.vetsgov-internal/

@thejordanwood
Copy link

thejordanwood commented Dec 7, 2023

@chriskim2311

  • The left side of Search should align with the left header content. The right side of Top Pages should align with the right side of the create account box.
  • I think we need a bit more spacing on the right side of Search and left side of Top Pages. The spacing can be equal at 32 px each. (Let's try that to see how it looks, it can be tricky trying to determine the pixels in Sketch v a live design.)
  • I also think there should be more room between the 2 link columns in Top Pages. Let's try 32 px there too.
  • This spacing should only apply to tablet sizes and desktop should stay the same. I say that because it looks like the spacing is changed in the PR environment for desktop.
  • My mockup is at 800 px for reference.

I also want to get @aklausmeier's opinion on this, since this is on the homepage and pretty visible.

Screenshot 2023-12-07 at 4 43 49 PM

@aklausmeier
Copy link

@chriskim2311 I agree with @thejordanwood's spacing suggestions on her 800px annotated mockup.

@chriskim2311
Copy link
Contributor Author

@thejordanwood @randimays @aklausmeier
New Screenshots at 800px. I have handled both the initial ask of the search bar touching at certain widths and added more space for the links in the top pages section. Let me know if you see any other necessary changes thanks!

Before:
VA_gov_Home___Veterans_Affairs

After:

VA_gov_Home___Veterans_Affairs

@randimays
Copy link
Contributor

@chriskim2311 I think there are still some spacing issues to address per Jordan's comment.

Here are some screenshots at a width right above the 768px breakpoint. Notice the search bar is creeping outside of its intended margin. There also should be 32px of padding on either side of the blue/white split in the middle per Jordan's comment.

Screenshot 2023-12-11 at 10 39 35 AM Screenshot 2023-12-11 at 10 39 30 AM

@thejordanwood
Copy link

@randimays @aklausmeier

@chriskim2311 and I met about this to talk through the spacing. It looks like the search bar has a minimum width, which prevents us from using 32px of spacing at smaller screen sizes. I'm fine with the smaller spacing between the Search and Top pages sections shown in Chris' recent screenshot.

The original problem for this ticket has been solved and the search bar no longer touches the edge of the screen on the left side at certain screen widths. With that in mind, this is approved by me but I would like @aklausmeier's final approval on this before merging.

Copy link
Contributor

@randimays randimays left a comment

Choose a reason for hiding this comment

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

Code LGTM; when design gives the green light I think we're good to go!

@aklausmeier
Copy link

@thejordanwood @randimays @chriskim2311 This is approved on my end!

…ent-build into 15967-homepage-common-tasks-css-fix
@chriskim2311 chriskim2311 merged commit 42fcb10 into main Dec 12, 2023
22 checks passed
@chriskim2311 chriskim2311 deleted the 15967-homepage-common-tasks-css-fix branch December 12, 2023 20:07
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.

6 participants