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

Backport additional search styles from brochure site #259

Merged
merged 1 commit into from
Jan 24, 2022

Conversation

aduth
Copy link
Member

@aduth aduth commented Oct 6, 2021

  • Full button text "Search" is always shown, regardless of viewport size
  • Reduce padding for default search button from 2 units (1rem) to 1.5 units (0.75rem)
  • Reduce font size for default search button from xs (1rem) to 2xs (0.87rem)

Live preview: https://federalist-2f194a10-945e-4413-be01-46ca6dae5358.app.cloud.gov/preview/18f/identity-style-guide/aduth-backport-more-search/components/search/

@aduth aduth requested review from nickttng and jmdembe October 6, 2021 17:48
@aduth
Copy link
Member Author

aduth commented Oct 6, 2021

The 2xs font-size token produces a value of 0.87rem / 13.92px, which is very close to--but not identical to--the Figma designs' 0.875rem / 14px. I wouldn't have thought this would have much an impact, but it does seem noticeable to me in practice. While I'd generally prefer deferring to a standard design system token, we could make an exception here and hard-code this to 0.875rem size?

0.87rem, Public Sans 0.875rem, Public Sans
image image

@nickttng
Copy link
Member

nickttng commented Oct 6, 2021

While I'd generally prefer deferring to a standard design system token, we could make an exception here and hard-code this to 0.875rem size?

@aduth Could you share more about why you'd like to make an exception here? Is there a case for deferring to a standard DS token?

@aduth
Copy link
Member Author

aduth commented Oct 6, 2021

While I'd generally prefer deferring to a standard design system token, we could make an exception here and hard-code this to 0.875rem size?

@aduth Could you share more about why you'd like to make an exception here? Is there a case for deferring to a standard DS token?

Personally, I would prefer to use the design system token, but doing so will require a slight visual change from how we currently show it in the brochure site and in the Figma, where it will appear slightly smaller (14px to 13.92px). If we're okay to make this change, I think it would be good to use the token instead.

@nickttng
Copy link
Member

nickttng commented Oct 6, 2021

Okay with making the change to use the design system token.

@aduth
Copy link
Member Author

aduth commented Oct 6, 2021

Okay with making the change to use the design system token.

Cool, that's how it was implemented here already, so no additional revisions should be necessary. 👍

Preview at : https://federalist-2f194a10-945e-4413-be01-46ca6dae5358.app.cloud.gov/preview/18f/identity-style-guide/aduth-backport-more-search/components/search/

@nickttng
Copy link
Member

nickttng commented Oct 6, 2021

Appreciate you noticing those kinds of details. To tie up some loose ends, it looks like the search component, both default and big variant, in the Figma library should be updated to reflect the LGDS, correct? Or at least note it.

@aduth
Copy link
Member Author

aduth commented Oct 6, 2021

To tie up some loose ends, it looks like the search component, both default and big variant, in the Figma library should be updated to reflect the LGDS, correct? Or at least note it.

Yeah, I expect so. Would you be able to help with that, or point me in the right direction? I can add some notes to the Figma document if it's useful to keep track.

I don't think there should be any revisions necessary for the "Big" variant. The styles added in this pull request exist only to make sure that the padding / font size we add to the base class don't apply for the big version. Essentially, it's a copy-paste of the default big styling for those values.

@nickttng
Copy link
Member

nickttng commented Oct 6, 2021

Yep, I'm able to help with that, and will tag you in Figma when updates are made - To make sure that LGDS and the library are both aligned.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

<label class="usa-sr-only" for="search-field-header-nav">Search</label>
<input class="usa-input" id="search-field-header-nav" name="query" type="search">
<form class="usa-search" role="search">
<label class="usa-sr-only" for="tkBH">Search</label>
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not used to these randomized IDs, is that a new thing we're doing?

Copy link
Member Author

Choose a reason for hiding this comment

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

i'm not used to these randomized IDs, is that a new thing we're doing?

It predates me, I've just kept it going 🤷 We would want unique IDs at least for each demo example, though I'm not sure the best way to include that in code snippets. USWDS goes the route of creating unique-but-meaningful IDs (example), which I think would work a bit better, though require more (up-front) effort. I've found these randomized IDs to be a bit of a pain actually, since I don't know that there's an easy way to enforce it, or even how best to create new ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah the red code in the diff had the meaningful ones so that's why I was wondering why the random ones came back

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah the red code in the diff had the meaningful ones so that's why I was wondering why the random ones came back

Ah, I see now. I might still be in favor of the revisions, since:

  • The IDs were copied verbatim from the brochure site. "heading" doesn't really apply in the context of this example.
  • Consistency for consistency's sake

- Full button text "Search" is always shown, regardless of viewport size
- Reduce padding for default search button from 2 units (1rem) to 1.5 units (0.75rem)
- Reduce font size for default search button from xs (1rem) to 2xs (0.87rem)
@aduth aduth force-pushed the aduth-backport-more-search branch from 80a159f to bb338f5 Compare January 24, 2022 20:15
@aduth aduth merged commit 95ce6eb into main Jan 24, 2022
@aduth aduth deleted the aduth-backport-more-search branch January 24, 2022 20:23
@aduth aduth mentioned this pull request Jan 25, 2022
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.

3 participants