Skip to content

Conversation

@amyleadem
Copy link
Contributor

@amyleadem amyleadem commented Aug 30, 2022

Summary

Removed redundant instructions for setting up the Sass file structure from the settings page.

Related issue

Closes #1701

Recommend releasing this update with the Phase 2 compile update in PR #1766

Preview link

Preview link: Settings page

Problem statement

Providing the same instructions in two different locations can cause confusion for the user and requires double maintenance. Additionally, users looking to configure their settings will receive more information than needed.

Solution

Replacing the instructions for setting up Sass files with a link to the Getting Started guide allows the user to focus on only configuring the settings.

Testing and review

To review:

  • Proofread for errors
  • Confirm that instructions makes sense
  • Confirm that instructions are comprehensive

Before opening this PR, make sure you’ve done whichever of these applies to you:

  • Confirm that this code follows the 18F Front End Coding Style Guide and Accessibility Guide.
  • Run git pull origin [base branch] to pull in the most recent updates from your base and check for merge conflicts. (Often, the base branch is develop).
  • Run npm test and confirm that all tests pass.
  • Run your code through HTML_CodeSniffer and make sure it’s error free.

@amyleadem amyleadem marked this pull request as ready for review September 1, 2022 14:05
@amyleadem
Copy link
Contributor Author

@bonnieAcameron @mejiaj
For this issue, I discovered that the instructions on the settings page had a lot of overlap with Phase 2, Step 1 of the getting started guide. I took a stab at restructuring the content for clarity so that the focus is entirely on settings configuration here, then point users to file setup instructions if they need it.

I know this is larger than the scope of the original issue, so let me know if the solution should be reined in. Let me know if you have any thoughts/opinions!

Hi Amy! This looks great! I added a couple of clarity suggestions and one grammatical update, but overall this is a super clear update, and definitely needed.
Copy link
Contributor

@bonnieAcameron bonnieAcameron left a comment

Choose a reason for hiding this comment

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

Hi Amy! I edited the file with a few of my suggestions & a grammar update, but this is an excellent update to the settings page.

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.

Nice job! This introduction seems clear and straightforward. Added some comments and questions on a few small things.

Co-authored-by: James Mejia <james.mejia@gsa.gov>
Copy link
Contributor

@bonnieAcameron bonnieAcameron left a comment

Choose a reason for hiding this comment

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

Looks great! I've reviewed for style guide, typos, and accessibility and have a few small changes to suggest:

Line 25-If we're specifically talking about the USWDS Design System, then we capitalize it: the Design System
Line 25, 44-instead of saying "tables below," (directional language isn't best practice for accessibility) can we use active voice to say, you can find USWDS settings and their default values in the USWDS settings tables

@amyleadem
Copy link
Contributor Author

Line 25-If we're specifically talking about the USWDS Design System, then we capitalize it: the Design System
Line 25, 44-instead of saying "tables below," (directional language isn't best practice for accessibility) can we use active voice to say, you can find USWDS settings and their default values in the USWDS settings tables

@bonnieAcameron Great feedback! I've updated these items in a5c672f

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.

Good changes, I like the added clarity. Thanks @amyleadem!

Copy link
Contributor

@bonnieAcameron bonnieAcameron left a comment

Choose a reason for hiding this comment

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

Reviewed for clarity, plain language (within reason) & typos.

Had a few ideas/clarifications, but other than that, fantastic changes.

Amy Leadem added 3 commits September 16, 2022 12:48
@amyleadem
Copy link
Contributor Author

@bonnieAcameron
Thanks for the notes! They helped me back up and make fewer assumptions.

I added some items to clarify in these commits: 2073839 and 840bffd.

  • Restructured the Where to include your configuration section for clarity
  • Replaced Example 1 and Example 2 with more descriptive names
  • Added more information up top about where this configuration should go
  • Added links to relevant Sass pages so users can get a deeper understanding if desired (I am guessing at least some users are still getting the hang it)

Copy link
Contributor

@bonnieAcameron bonnieAcameron left a comment

Choose a reason for hiding this comment

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

Looks great, Amy!

Copy link
Contributor

@thisisdano thisisdano left a comment

Choose a reason for hiding this comment

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

Nice improvement here

@thisisdano thisisdano merged commit d025e87 into main Sep 26, 2022
@thisisdano thisisdano deleted the al-settings branch September 26, 2022 16:04
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 - Bug: update settings documentation

5 participants