Skip to content

Conversation

@olitharp-nci
Copy link
Contributor

@olitharp-nci olitharp-nci commented Oct 7, 2024

Pull Request Details

  • Replaces webpack 4 with vite
  • Changes commonjs file extensions to .cjs from .js
  • Touches uswds search, paginagtion, collection, and icon list twig files
  • Adds ncids webfonts to css - to help with loading issues when rendering backstop
  • Fixes @use when it should be @forward and reference the package name correctly
  • Setup static folders in main.js
  • Wait for network requests to be done before starting tests.
  • Removed footer form cgov_home content
  • Removed page options from template without side nav to match current

Closes #1421

Author PR Checklist

Items that the author of the PR is responsible for checking before submitted the PR.

General:

  • I have reviewed the acceptance criteria defined in the ticket and ensured the work has been completed.
  • The commit message passes all quality commit message standards.
  • Unit tests have been updated or created to reflect any javascript changes.
  • Storybook scenarios have been updated or created to reflect any html/css/js changes.

Accessibility:

  • WCAG 2.1 Level AA requirements have been met.

Development:

  • Any new or updated javascript code has 100% unit test coverage.
  • New or updated breakpoints have regression images.
  • Breaking changes have been thoroughly documented in the PR.

Product Reviewer PR Checklist

Items the product team is responsible for reviewing.

General:

  • There are no unexpected or unapproved regression image changes.

  • Functionality of interactive elements meet the acceptance criteria.
  • The product is visually and functionally the same across the different browsers.

Accessibility:

  • AxeDev Tools: there are no new or outstanding accessibility issues introduced in this PR.
  • Lighthouse: scores have not noticeably decreased during this PR.
  • Wave: there are no new errors or contrast errors introduced in this PR.

Design Reviewer PR Checklist

Items the design team is responsible for reviewing. 


General:

  • New or updated features introduced in this PR are developed mobile-first.
  • Breakpoint changes and regression images match those breakpoints.
  • This PR has been tested in all supported browsers at all breakpoints.

Developer Reviewer PR Checklist

Items the development team is responsible for reviewing.

General:

  • New code passes code quality standards set by industry standards.
  • The expected Storybook stories have been added or updated for the new or updated feature.
  • The expected unit tests have been added or updated for the new or updated feature.

Accessibility:

  • VoiceOver: Described content matches with what was expected.
  • Keyboard navigation: new or updated features and content are navigable via the keyboard.

@github-actions
Copy link

github-actions bot commented Oct 7, 2024

@olitharp-nci olitharp-nci force-pushed the ticket/1421-storybook-vite branch 3 times, most recently from ac3a149 to 3cd1d5c Compare October 14, 2024 17:18
@olitharp-nci olitharp-nci force-pushed the ticket/1421-storybook-vite branch 2 times, most recently from fe6dda6 to f46157d Compare October 30, 2024 21:15
@olitharp-nci olitharp-nci force-pushed the ticket/1421-storybook-vite branch from fe59139 to a881f97 Compare November 8, 2024 20:49
@olitharp-nci olitharp-nci force-pushed the ticket/1421-storybook-vite branch from b59682c to a8a136a Compare November 19, 2024 18:50
@olitharp-nci olitharp-nci force-pushed the ticket/1421-storybook-vite branch from 8ec9f17 to a6da848 Compare November 26, 2024 22:31
@bryanpizzillo bryanpizzillo force-pushed the ticket/1421-storybook-vite branch from 837bcb4 to 04d7b13 Compare November 26, 2024 23:55
@olitharp-nci olitharp-nci force-pushed the ticket/1421-storybook-vite branch 5 times, most recently from 7016166 to dd0b303 Compare December 3, 2024 01:33
@bryanpizzillo
Copy link
Member

bryanpizzillo commented Dec 3, 2024

Something between the Docker container were tests run, the new Storybook and the Site-wide search text input has introduced a visual difference. The text input is now a bit wider in screen captures. We can see this both in GitHub Actions and locally running within a container. The newest Backstop does not seem to fix it. This difference cannot be seen in the browser when comparing the existing header stories against the updated storybook stories. (They both look exactly the same)

Here is what the bad looks like:
image

Additionally, testing locally, without Docker, produces the following comparison which shows the two images as basically the same. Running on a Mac vs Docker/Linux will always have slight visual differences, which is why we use Docker so that is why there is a lot of magenta in the diff.
image

So for now, we are just going to have to accept the updates to the reference images that show the input as a little longer. This means that from a visual regression standpoint any changes accidentally introduced should still cause some change to the input, which would be then be confirmed on the front-end.

There will probably be some edge case where a change to the input would yield the same image with the wider input and pass tests, but visually in a browser would have differences. The chance of this happening will be very low and acceptable given the time spent trying to debug this issue.

@olitharp-nci olitharp-nci force-pushed the ticket/1421-storybook-vite branch 2 times, most recently from 77217a0 to 57f856f Compare December 3, 2024 16:55
@olitharp-nci olitharp-nci marked this pull request as ready for review December 3, 2024 17:11
@olitharp-nci olitharp-nci requested review from a team as code owners December 3, 2024 17:11
@olitharp-nci olitharp-nci force-pushed the ticket/1421-storybook-vite branch from 57f856f to 1e73c07 Compare December 4, 2024 16:49
@olitharp-nci olitharp-nci force-pushed the ticket/1421-storybook-vite branch from 1e73c07 to 97973f5 Compare December 4, 2024 17:11
@olitharp-nci
Copy link
Contributor Author

pushes after dev review were only some clean up things and also i wanted to run the backstop workflow a couple more times anyway

- Replaces webpack 4 with vite
- Changes commonjs file extensions to .cjs from .js
- Touches uswds search, paginagtion, collection, and icon list twig files
- Adds ncids webfonts to css - to help with loading issues when rendering backstop
- Fixes @use when it should be @forward and reference the package name correctly
- Setup static folders in main.js
- Wait for network requests to be done before starting tests.
- Removed footer form cgov_home content
- Removed page options from template without side nav to match current

Closes #1421
@olitharp-nci olitharp-nci force-pushed the ticket/1421-storybook-vite branch from 97973f5 to 87c62c3 Compare December 4, 2024 19:53
Copy link

@andyvanavery31 andyvanavery31 left a comment

Choose a reason for hiding this comment

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

Passes Product review.

@olitharp-nci olitharp-nci merged commit d3f7c52 into develop Dec 4, 2024
3 checks passed
@olitharp-nci olitharp-nci deleted the ticket/1421-storybook-vite branch December 4, 2024 20:18
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.

Enabler: Update ncids-css-testing Storybook to version 7

6 participants