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

Remove "Usage" section of design.login.gov #228

Merged
merged 8 commits into from
Jul 16, 2021
Merged

Conversation

juliaelman
Copy link
Member

This pull request transitions "Usage" of the design system from living on design.login.gov as a .md file to being part of the README. As the user is primarily internal team members, it makes sense to keep this installation and usage information in one place (instead of two).

@juliaelman juliaelman requested a review from aduth July 13, 2021 15:54
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

docs/usage.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
@@ -81,3 +89,94 @@ Before starting, make sure that all changes intended for release should be merge
- The release version should match the version just published to `npm` (for example, `v2.1.5`).
- Use the version name as the release title.
- Use the release notes to link to any important issues or pull requests that were addressed in the release. You may copy this from `CHANGELOG.md`.

## Use with Rails
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to group the two "Use with/as" headings under a "Usage" heading? Maybe also the Jekyll plugin if we reincorporate it per Zach's comment.

e.g.

## Usage

...

### Use with Rails

...

### Use as a JavaScript package

...

Copy link
Member Author

Choose a reason for hiding this comment

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

I dig it 20933ae

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
aduth and others added 7 commits July 16, 2021 12:13
* Remove custom Accordion close button element

**Why**:

Maintenance:
- Limit divergence from USWDS, facilitating ongoing maintenance and reducing potential confusion introduced by enhancements exclusive to the login.gov Design System.

Accessibility:
- The button's behavior is duplicative with the toggle, where the additional tab stop may be excessive as repeated content.
- Collapsing an accordion by the close button causes a focus loss ([SC 2.4.7](https://www.w3.org/TR/UNDERSTANDING-WCAG20/navigation-mechanisms-focus-visible.html), [SC 2.4.3](https://www.w3.org/WAI/WCAG21/Understanding/focus-order.html))
- The relationship between the button and controlled content is not clearly indicated (e.g. [`aria-controls`](https://www.w3.org/TR/wai-aria-1.1/#aria-controls))

* Remove unused variable $icon-size

* Restore border radius to accordion content bottom border

* Clarify removal of JavaScript exported member
Co-authored-by: Andrew Duthie <[email protected]>
Co-authored-by: Andrew Duthie <[email protected]>
Co-authored-by: Andrew Duthie <[email protected]>
Co-authored-by: Andrew Duthie <[email protected]>
@juliaelman juliaelman requested a review from aduth July 16, 2021 16:21
@juliaelman
Copy link
Member Author

@aduth updates made. Re-requested review before 🚀 please 😄

Copy link
Member

@aduth aduth 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 👍

("Files changed" look like they include changes from #229, but pretty sure that should merge cleanly)

@juliaelman juliaelman merged commit f496128 into main Jul 16, 2021
@juliaelman juliaelman deleted the je-remove-usage-page branch July 16, 2021 17:42
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.

3 participants