feat: Update GovBanner to match USWDS 2.8.0 release

[brandonlenz]

Summary

Related Issues or PRs

closes #456

How To Test

Verify copy changes in Storybook for .gov and .mil versions of the banner for both English and Spanish

Screenshots (optional)

Storybook stories:

defaults to "english" & ".gov":

with "english" and ".gov" passed:

with "english" and ".mil" passed:

with "spanish" and ".gov" passed:

with "spanish" and ".mil" passed:

[brandonlenz]

Warnings
⚠️ This PR does not include changes to tests, even though it affects source code.
Generated by 🚫 dangerJS against f8db555

I'm curious what reviewers think about for testing this story.

I'm cautious to test against specific verbiage being rendered, as that makes for tests that are low in value in terms of likelihood in catching breaking changes, but do make future updates disproportionally more cumbersome. Using something like Enzyme would allow testing of actual property values, which would allow us to test default values for props when not explicitly passed. What are your thoughts on that?

In 7efcce7 I added a basic test verifying render with the newly added props. I'd be happy to expand on that by testing the actual verbiage if maintainers agree there is value in that kind of test.

[sojeri]

@suzubara / @haworku --- I didn't make it all the way through the happo diff, but is it normal to see so many false positives? when I saw the uswds bump I thought that might be the trigger, but switching to diff view the vast majority of components seem to have no visible changes. any idea why these are showing seemingly empty diffs or whether that's a normal behavior here?

literally the only change I saw in the components I reviewed was the warning icon changing ever so slightly.

[brandonlenz]

@suzubara / @haworku --- I didn't make it all the way through the happo diff, but is it normal to see so many false positives? when I saw the uswds bump I thought that might be the trigger, but switching to diff view the vast majority of components seem to have no visible changes. any idea why these are showing seemingly empty diffs or whether that's a normal behavior here?

literally the only change I saw in the components I reviewed was the warning icon changing ever so slightly.

@sojeri I meant to address that here as well with my understanding of a lot of it. Indeed, it looks like some of the icons may have changed ever so slightly. I also noticed the note at the top of Happo:

Some updates were made to Happo workers that might cause unexpected diffs:
1/11/2021, 5:08:00 AM - Our Chrome workers have been updated to run Chrome version 88.

Many of the diffs were from Chrome.

There are some valid diffs from the 2.8.0 upgrade, for instance, the proper alignment of the collapse-icon for the GovBanner:

All that said, I am new to Happo, so I'm not sure what is normal to see.

[haworku]

In 7efcce7 I added a basic test verifying render with the newly added props. I'd be happy to expand on that by testing the actual verbiage if maintainers agree there is value in that kind of test.

Thanks for asking about tests, @brandonlenz ! imo we should add a basic test for the case where these new props are not passed in at all. That is how most people using this component right now out in the wild (since this is new work). The component should still render with the English text and not break for those folks.

I hear you on Enzmye, sounds like a bigger topic we could discuss. Want to make an issue about it? I don't think this should block this work from going through now. This def an outlier component because all it does really is display very specific text which we try to avoid (#124).

I didn't make it all the way through the happo diff, but is it normal to see so many false positives?

@sojeri Um... sometimes. 😇 Lately Happo has been catching bugs for us and has been pretty on point. However, this does look like a bunch of false positives maybe related to what you all already identified. Especially when its just a few pixels change for unrelated components I usually push it through. Its approved now. ✅

[brandonlenz]

In 7efcce7 I added a basic test verifying render with the newly added props. I'd be happy to expand on that by testing the actual verbiage if maintainers agree there is value in that kind of test.

Thanks for asking about tests, @brandonlenz ! imo we should add a basic test for the case where these new props are not passed in at all. That is how most people using this component right now out in the wild (since this is new work). The component should still render with the English text and not break for those folks.

@haworku The existing test for rendering the component without props should work for this purpose:

  it('renders without errors', () => {
    const { queryByTestId } = render(<GovBanner />)
    expect(queryByTestId('govBanner')).toBeInTheDocument()
  })

I hear you on Enzmye, sounds like a bigger topic we could discuss. Want to make an issue about it? I don't think this should block this work from going through now. This def an outlier component because all it does really is display very specific text which we try to avoid (#124).

Sure, I'll create an issue to start discussion on that. I can definitely see an argument being made to keep this repo lean.

Also thanks for linking #124. I definitely agree this could be slippery slope territory. For the purposes of this PR, if we only want to consider english for now I can remove the logic specific to "spanish", and add optional props in the spirit of

it sounds like we have agreement about using props (with English default values) to handle any necessary language strings (#124 (comment))

Otherwise I could see someone taking a look at what I've done here and implementing similar logic in the name of convention elsewhere.

[brandonlenz]

Sure, I'll create an issue to start discussion on that. I can definitely see an argument being made to keep this repo lean.

Added #783

[haworku]

🏅 Thanks for making the Enzyme issue as well.

I think this implementation and the way props are handled make sense for the use case. It's an exception to the rule, yes, and we always have those. Good work

[suzubara]

This looks awesome! I found one typo that should be resolved before merging. I think it would be good to consider a future abstraction where each of the copy values can be passed in as props in order to easily allow for other localizations, but since the USWDS spec only explicitly provides copy for English & Spanish I think this is sufficient.

as for the testing question, I think this might be a good situation to use Jest snapshot testing? I agree writing RTL tests that look for specific copy will be tedious to maintain, and this component is unique in that it's purpose is to render specific content. I know we already get visual tests from Happo, but if we want more confidence that this component will not change unexpectedly, snapshot tests would be my suggestion.

[suzubara]

typo: Offical > Official

[brandonlenz]

Good eye! 1378092

[brandonlenz]

Added snapshot tests in d8b3a91

I hadn't actually used those before, so this was a good learning experience. Please verify what I ended up doing is in line with convention for Snapshot tests at your convenience 😄

[trussworks-infra-zz]

	Warnings
⚠️	It looks like there are new component (JSX/TSX) files, but the entrypoint (index.ts) has not changed. - Did you forget to export new components from the library entrypoint?

Generated by 🚫 dangerJS against 2f46e60

[suzubara]

awesome, it's a ✅ from me!