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

USWDS-Site - Migration guide: Add note about autoprefixer #2319

Merged
merged 17 commits into from
Jun 7, 2024

Conversation

mahoneycm
Copy link
Contributor

@mahoneycm mahoneycm commented Oct 25, 2023

Summary

Moved Autoprefixer note to an include and added the note to the Migrating to 3.0 page.

Warning
Ignored two snyk vulnerabilities causing test failures. This can be resolved by #2322

Warning resolved and can be ignored. Leaving for historical context.

Related issue

Closes #2296

Preview link

Migrating to 3.0 page →
Phase 2: Compile →

Changelog →

Note
Must update changelog date before merge

Problem statement

Issue arose from a discussion in the public slack channel. Some users may be using the migration guide exclusively to set up their builds. Without this important piece of information, these users may run into issues with their CSS.

Solution

Use note about Autoprefixer from Phase 2: Compile page in the Sass compiler instructions.

Testing and review

  1. Inspect new autoprefixer-note.html does have gramattical errors.
  2. Visit the Phase 2: Compile page
  3. Confirm Autoprefixer note matches what's on develop.
  4. Visit the Migrate to 3.0 page
  5. Inspect new note re: Autoprefixer
  6. Confirm placement is appropriate

Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

This looks good to me! The content makes sense and was added to a logical location. I had just a couple notes:

I wonder if it makes sense to create an include for this content since it appears to be duplicate content from the Getting started for devs guide. It isn't clear what our pattern is on the site, but creating a shared include would ensure consistency and simplify editing if we ever wanted to change the content there. Curious what you think.

Also, can you resolve the merge conflicts?

@mahoneycm
Copy link
Contributor Author

@amyleadem Good thinking! Changed pushed up and conflicts resolved!

Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Looks good to me!

  • Confirmed that the site note on the compile page is unchanged
  • Confirmed that the migration guide now has a note about autoprefixer
  • Confirmed the note on the migration page is in a logical location
  • Confirmed the note on the migration page has content that makes sense in context
  • Confirmed the changelog is accurate and makes sense

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

LGTM, changelog date and preview link for phase-two needs to be updated. Good to merge after.

@mahoneycm
Copy link
Contributor Author

@mejiaj I've updated the changelog date and phase-two link. Pa11y tests are failing in the pipeline but working passing!

@mahoneycm
Copy link
Contributor Author

@thisisdano quick win PR ready for your review!

@thisisdano thisisdano merged commit 6f1a1b5 into main Jun 7, 2024
11 checks passed
@thisisdano thisisdano deleted the cm-migration-autoprefixer branch June 7, 2024 19:30
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.

USWDS-Site: Add note about Autoprefixer to migration guide
4 participants