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

fix!: Unify icon usage #2411

Merged
merged 9 commits into from
May 31, 2023
Merged

fix!: Unify icon usage #2411

merged 9 commits into from
May 31, 2023

Conversation

werdnanoslen
Copy link
Contributor

@werdnanoslen werdnanoslen commented May 22, 2023

Summary

Addresses these two ACs in the original issue:

  • Double check our components are using the correct uswds icons. GovBanner uses a lock icon for example.
  • Replace usage of icons in our components with our react-uswds .

Related Issues or PRs

Resolves #1303

How To Test

Confirm that all SVGs still in use are for images that aren't icons

Screenshots (optional)

N/A, looks the same visually, and behaves the same with some attribute changes

@werdnanoslen werdnanoslen changed the title Unify icon usage fix: Unify icon usage May 22, 2023
@brandonlenz
Copy link
Contributor

brandonlenz commented May 23, 2023

There's some Happo diffs, in particular SocialLinks which I think needs adjustment before merge:

image

The icon used in the search component reduces the size pretty noticeably from what we previously had, which I think deviates far enough from the USWDS examples to call out (Looks like its a safari thing though 🤔):

image

Since this PR removes a consumer-facing component, it'll be a breaking change. Nothing to worry about with that, but you should update the title to add a ! after fix so that our release process properly bumps our major version on the next release based on the conventional commit spec

@werdnanoslen
Copy link
Contributor Author

werdnanoslen commented May 23, 2023

Social links I wasn't sure if it needed to change per se, given the styles referenced there turn it black based on its parent div (medium/big footers) and USWDS site doesn't define their style directly. So since our implementation uses their styles, they are sort of 'supposed' to be blue out of their context, since they're links. I think 🤷

image

I'll fix the mobile search icon bug though, I left out the size attr

@werdnanoslen werdnanoslen changed the title fix: Unify icon usage fix!: Unify icon usage May 23, 2023
@werdnanoslen
Copy link
Contributor Author

@brandonlenz are Happo diffs approvable?

Copy link
Contributor

@gidjin gidjin left a comment

Choose a reason for hiding this comment

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

LGTM, left a question but it's not blocking.

@@ -40,7 +40,9 @@ describe('SocialLinks component', () => {
describe('SocialLink component', () => {
it('renders without errors', () => {
const { container } = render(<SocialLink name="Instagram" />)
expect(container.querySelector('.usa-social-link')).toBeInTheDocument()
expect(screen.getByRole('img')).toHaveAttribute('alt', 'Instagram')
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Should these have alt tags too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, the alt prop only applies to <img>. Now that it's an <svg>, I have a line at the bottom of that block to check the name prop instead, which serves the same purpose.

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.

[feat] Unify icon usage
3 participants