-
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 - Date range picker: Add date picker known issues to date range picker #2620
Conversation
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.
Note
Edited the vars here to accomplish the following:
- Accommodate the new
known_issues_keyin one spot - Have clearer names (
issues->known_issues) - Standardize implementation and remove duplicate code for
know-issues-alert.htmlandknown-issues.html
|
|
||
| {%- if issues.items -%} |
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.
Note
I moved this check to component.html to limit code duplication
| @@ -55,4 +55,5 @@ title: Date range picker | |||
| type: component | |||
| changelog: | |||
| key: component-date-range-picker | |||
| known_issues_key: date-picker | |||
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.
Note
Open to suggestions for naming this data key.
| @@ -1,17 +1,15 @@ | |||
| {% assign page_name = page.title | downcase %} | |||
| {% assign page_slug = page_name | slugify %} | |||
| {% assign github_label = page.github_label_url | default: page.title | replace: ' ', '+' %} | |||
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.
Note
github_label_url is not currently used. Eventually, we might need to bring this back because the github label names do not always line up with the component names, but I would prefer updating the label name instead. We can explore this later on if needed.
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.
This looks good to me! Left one comment about a potential future enhancement to allow multiple pages issues to be combined.
- Confirm that the date picker known issues appear on the date range picker page
- Confirm that all links in the known issues section point to the expected location
- Confirm that the known issues alert appears at the top of the date range picker page
- Confirm no regressions on other pages with known issues (character count, combo box, date picker, file input, input mask, validation)
- Confirm this code meets standards
| @@ -2,6 +2,12 @@ title: Date range picker | |||
| type: component | |||
| changelogURL: | |||
| items: | |||
| - date: NNNN-NN-NN | |||
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.
Note
We need to update this changelog date before merge
…te-range-picker-known-issues
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.
Overall looks good. Added minor, optional, suggestions for clarity.
| @@ -2,6 +2,12 @@ title: Date range picker | |||
| type: component | |||
| changelogURL: | |||
| items: | |||
| - date: NNNN-NN-NN | |||
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.
| - date: NNNN-NN-NN | |
| - date: 2024-04-30 |
_includes/known-issues-alert.html
Outdated
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
Create a reusable alert component.
uswds-site/_includes/component.html
Lines 8 to 17 in a968712
| {% if page.alert == true %} | |
| <div class="usa-alert margin-bottom-3 usa-alert--{{ page.alert-class }}"> | |
| <div class="usa-alert__body"> | |
| <h2 class="usa-alert__heading">{{ page.alert-heading }}</h2> | |
| <p class="usa-alert__text"> | |
| {{ page.alert-content }} | |
| </p> | |
| </div> | |
| </div> | |
| {% endif %} |
Co-authored-by: James Mejia <james.mejia@gsa.gov>
Summary
known_issues_keyfront matter variable so that we can share known issues across pages if needed.Related issue
Closes #2580
Preview link
Date range picker page
Problem statement
The date range picker is affected by known issues listed on the date picker page. The date range picker page should pull in known issues from the date picker component.
Solution
Testing and review