department-of-veterans-affairs / vets-design-system-documentation

Repository for design.va.gov website
https://design.va.gov
36 stars 55 forks source link

[Header] Mobile template and header component in Sketch library do not match va.gov mobile view #662

Closed GnatalieH closed 2 years ago

GnatalieH commented 2 years ago

Bug Report

What happened

This is the template that currently lives in Sketch: Sketch template

What I expected to happen

The Sketch assets should match the current mobile header style.

Screen Shot 2022-05-02 at 4.59.37 PM.png

Steps to resolve this issue:

Mockups

Link to new mobile header mockups in Sketch for Teams.

humancompanion-usds commented 2 years ago

@GnatalieH - Here is my feedback:

Please double check the paddings and heights within the mobile header. Specifically, when I select the "VA" text it's 13px rather than 12px from the left edge. I think that text box should also be resized. I'd like anyone using the symbol to be able to use the feature in Sketch to hold down Option and see the padding in px on all 4 sides. (If I'm doing a lousy job of explaining let's just do a quick screen share; it's easier to show than describe)

We have clean-up to do with Symbol naming. I would like to start moving towards the draft IA. Thus the header should live under Navigation (which I see we already have). Here is what I propose:

Navigation / header / mobile / authenticated Navigation / header / mobile / unauthenticated Navigation / header / desktop / authenticated Navigation / header / desktop / unauthenticated

Navigation / header / mobile / _elements / menu-button (unless this lives in buttons already?) Navigation / header / mobile / _elements / nav-item / accordion / closed Navigation / header / mobile / _elements / nav-item / accordion / open Navigation / header / mobile / _elements / nav-item / no-accordion Navigation / header / mobile / _elements / veterans-crisis-line

Navigation / banner / mobile / usa-banner (I know it's not actually the usa-banner from v2 of USWDS but we'll let that slide for the moment)

Eventually we'll want to add: Navigation / header / mobile / authenticated / MyVA Navigation / header / mobile / authenticated / Profile

Those are currently in the Authenticated Experience Pattern Library. But they look to also be out-of-date so I want to talk to that team about working together to bring those variations into the system.

Anyway, happy to chat about this if you have any concerns, questions, or alternative ideas.

GnatalieH commented 2 years ago

@humancompanion-usds regarding this point:

"Sign in" should be 12px to the right of the Menu button, it's 24px in Sketch.

Do you mean there should be 12px between the Sign in and the Menu button?

I understand what you're suggesting with the Option key, I use that a lot in Sketch.

For the expanded state, I used a lot of assets that Ryan had started (which may not have been updated since this header launched). I didn't catch all of the the size and spacing discrepancies you identified. Thanks!

humancompanion-usds commented 2 years ago

Oh, whoops, yes, the distance between Sign In and Menu (so right side of Sign In and left side of Menu) should be 12px. Cool, thanks for fixing those measurements up.

GnatalieH commented 2 years ago

@humancompanion-usds I'm not sure how to make Sketch elements in each header "row" have the same 4px padding on all sides when there is a combination of text and images (or text, images, and a button) in the row. It's easy to do with a text box only, but do you know how to make a group of objects work that way? The row with the VA logo is complicated by the fact that there's a mask being used. I'm happy to make an SVG logo, but I'm not sure how to do that from the logo SVG code on the site. Thanks in advance.

Edit: I can make most of the 4 px padding on all sides instances work by adding a shim rectangle shape in the Sketch objects. The VA logo row may require more than one shim.

Edit 2: Scratch that regarding my earlier edit. I am stumped with how to emulate the CSS padding using Sketch objects. I don't know if there's a way to make the Sketch file behave how the developed element works. Sorry, @humancompanion-usds, any suggestions?

GnatalieH commented 2 years ago

I'm suggesting we close this ticket as the original requirements are met, and create a follow up ticket to address the expanded state. I had estimated a "2" for this ticket based on updating what existed in the Sketch library previously, which was just the collapsed state of the header. @humancompanion-usds and @caw310 is this OK? I just want to make sure I can get all my tickets done this sprint, but I agree that including the expanded state is important. I do think incremental improvement of the updated collapsed state header could be an immediate benefit to Designers. Thanks.

caw310 commented 2 years ago

@GnatalieH I would say just keep this ticket open and we can extend it into the next sprint. I think it is possibly easier to track.

humancompanion-usds commented 2 years ago

It's fine to handle expanded state later but, I agree that we should just let the ticket go into the next sprint.

humancompanion-usds commented 2 years ago

I reviewed the Mobile Header symbols and they look solid. Only thing I found was that the Veterans Crisis Line is on a half-pixel distance from the left-edge. Let's avoid the .5 sub-pixel thing. Ship it!

GnatalieH commented 2 years ago

@humancompanion-usds I fixed the .5 pixel distance thing. The VCL row in this header is actually what I had the most trouble wrangling, oddly. I still am not quite sure how to simulate the CSS specs (4px on all sides, for example) in Sketch elements that contain multiple layers. Thanks!

humancompanion-usds commented 2 years ago

@GnatalieH - I'm not so worried about nested components. As long as I can drill down to the symbol and get the right measurements, that works. For the header my concerns would be making sure that pixel height of each sub-element is correct. Especially on mobile I want folks using the design system to build their prototypes to get an accurate idea of how much height the header (and eventually the nav) takes up.

humancompanion-usds commented 2 years ago

Reviewed this file: https://sketch.com/s/3cff1216-43e4-47be-933c-a62b640cf084

We need to create a checklist of things to verify. Top of my head:

I don't have enough time this morning to check each of these things but if you take another pass through and believe this is covered then move forward.

GnatalieH commented 2 years ago

@humancompanion-usds thanks for finding that typo! I'll check to make sure everything is OK. I think the pixel dimensions are good, but I'll double check when I go down your list.