-
Notifications
You must be signed in to change notification settings - Fork 148
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: Add component lifecycle pages #2491
Conversation
| USWDS is committed to building a safe, welcoming, harassment-free culture for everyone. USWDS is a part of the Technology Transformation Services (TTS) within the General Services Administration (GSA) and we expect everyone to follow the GSA TTS Code of Conduct. | ||
|
|
||
| We encourage you to read the [LICENSE](https://github.com/uswds/uswds/blob/develop/LICENSE.md) and its [README](https://github.com/uswds/uswds/blob/develop/README.md). If you want to read more about our open source policy or have questions, check out the [18F Open Source Policy GitHub repository](https://github.com/18f/open-source-policy) or send us an [email](mailto:uswds@gsa.gov). | ||
| We encourage you to read the [LICENSE](https://github.com/uswds/uswds/blob/develop/LICENSE.md) and the [LICENSE’s README](https://github.com/uswds/uswds/blob/develop/README.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sarah-sch The README link here seems to just point to the USWDS readme. The link is the same as what is currently on the site. Is this where it should point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amyleadem - Hm, I am not sure if there is another README specific to the license. We could point to the license section of the USWDS README, but it is not very extensive and just points people back to the License. @annepetersen and @thisisdano - any thoughts? If we get in a time crunch I think it is ok to publish as is and figure this out after the MC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there isn't a README specific to the license, we could just cut it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving and leaving some suggestions for your consideration, @amyleadem.
| ### Want to propose something else? | ||
| ## Want to contribute something else? | ||
| If you’d like to contribute something else that doesn’t fall into any of the above, we’d still love to hear about it. | ||
| Just [create an issue](https://github.com/uswds/uswds/issues/new/choose) or [create a discussion](https://github.com/uswds/uswds/discussions/new?category=component-proposals), and we can talk about it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amyleadem, do you think that from "create a discussion" we should link to https://github.com/uswds/uswds/discussions instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could! Initially I was thinking of just pointing them to start a new discussion, but it might be more informative your way. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR description
Changed the two !IMPORTANT notes as checklist items. Mainly to make it more scannable and show that there's additional work to be done.
PR review
Looks good visually and no A11Y issues found.
Added comments for improvements. Comments labelled as Future enhancement are meant as a follow-up. Mainly to improve maintainability, reduce code duplication, and moving data closer to where it lives.
| @@ -162,7 +162,8 @@ About: | |||
| - href: /about/ | |||
| - href: /about/key-benefits/ | |||
| - href: /about/product-values/ | |||
| - href: /about/contribute/ | |||
| - text: Contribute | |||
| href: /about/contribute/ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't the title in about/contribute.md be a better place for this?
uswds-site/pages/about/contribute.md
Line 2 in 341422c
| title: Contribute to USWDS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created this so that the sidenav link could stay as "Contribute". But it might not be necessary! Happy to remove if we are ok with the sidenav saying "Contribute to USWDS"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amyleadem I see, I wasn't aware we wanted to keep the page title as Contribute to USWDS. I was suggesting changing the title because it'd be consistent everywhere.
In that case, we can ignore this.
| } | ||
|
|
||
| .usa-prose > .lifecycle-table { | ||
| @include u-font("lang", "3xs"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future enhancement
This is pretty small, but it looks like other tables share small font size too.
Ex: /utilities/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future enhancement
Consider either:
- Moving this to frontmatter. This data is only used on the phases page.
- Moving to markdown to make it easier for others to update/maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future enhancement
- Consider splitting this data to make it easier to manage. Ex: Consider component frontmatter.
- Improve clarity in relationship between
status_majorandstatus_minor.- Are both required?
- What does one have to do with the other?
- What are all of the possible states?
- Use relative URLs for component pages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Toying around with the table classes, it looks like we might be able to consolidate these into one status and still be able to appropriately assign classes.
I removed the item.status_major checks that don't directly affect classes and didn't notice any visual regressions in the lifecycle table
Scratch that, I inspected desktop instead of mobile styles. I still suspect we can handle this with a single status but more testing required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should look at how the data structure relates to the table structure (or the element it feeds). For example, is it set up in a way that makes templating intuitive. If one relies on the other, but they're siblings should we convert to something else on data-side?
That could not only help us improve the data, but also how we create the template elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future enhancement
Move to own partial for easier maintenance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future enhancement
- Separate in to partial
- Explore ways to improve setting
status_classes. Ex: repetition on lines19-23,48-69,100-120.
Co-authored-by: James Mejia <james.mejia@gsa.gov>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future enhancement
I'd love to see how it'd be possible to manage one table instead of two.
One thing I think we could do is use a combination of media queries and column-based class styles applied to td's to set display: none. I briefly looked into and but I don't think it'll affect display correctly.
Might be tedious but we could add a collapsable-column class to the columns we want to hide and set styles accordingly. Maybe I should submit a component variant proposal... 🤔
…mponent-lifecycle-pages
_data/lifecycle-phases.yml
Outdated
| find_more: | | ||
| All experimental components are documented on our <a href="https://designsystem.digital.gov/components/overview/">components page</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thisisdano @annepetersen Wondering if I should remove this until we have experimental components available. For now, we could just delete it. But in the future I could probably find a way to do a check to see if we have any experimental components in the status data before showing.
| find_more: | | |
| All experimental components are documented on our <a href="https://designsystem.digital.gov/components/overview/">components page</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm tending more towards saying "In the future, all experimental components will be documented...", personally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the text to match your suggestion in 0610be8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NICE.
Addressed issues or moved into new issues
|
Merging over pa11y crashes |
Summary
Todo
uswds-proposalsRelated issue
Closes #2461
Preview links
Testing and review