-
Notifications
You must be signed in to change notification settings - Fork 177
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: Enhance what's new page layout #3115
base: al-whats-new-directory-change
Are you sure you want to change the base?
USWDS-Site: Enhance what's new page layout #3115
Conversation
…ats-new-layout-update
…wds/uswds-site into al-whats-new-layout-update
…wds/uswds-site into al-whats-new-layout-update
columns: | ||
- title: New users | ||
source: New Users | ||
- title: Total users | ||
source: Users | ||
- title: Page views | ||
source: Pageviews |
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 removed this because I could not find it being used anywhere.
changelog: | ||
key: about-whats-new |
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
Removed the page's changelog to make room for the consolidated changelog.
pages/whats-new/overview.md
Outdated
<div class="bg-gray-5 padding-3 margin-top-4"> | ||
<h2 class="whats-new__heading margin-top-0" id="latest-releases">Latest releases</h2> | ||
<ul class="usa-card-group whats-new-card-group"> | ||
<li class="{{ product_grid_list_classes }}"> | ||
{% include site-card.html | ||
subheading="Design system" | ||
heading="USWDS" | ||
body="The official U.S. design system, enabling the government to build fast, accessible, mobile-friendly websites." | ||
linkText=uswds_link_text | ||
linkUrl="https://github.com/uswds/uswds/releases" | ||
subheadingElement="span" | ||
linkClasses="padding-x-105" | ||
%} | ||
</li> | ||
<li class="{{ product_grid_list_classes }}"> | ||
{% include site-card.html | ||
subheading="Compilation tool" | ||
heading="USWDS Compile" | ||
body="A tool that makes it easy to customize and compile USWDS Sass into browser-readable CSS." | ||
linkText=uswds_compile_link_text | ||
linkUrl="https://github.com/uswds/uswds-compile/releases" | ||
subheadingElement="span" | ||
linkClasses="padding-x-105" | ||
%} | ||
</li> | ||
<li class="{{ product_grid_list_classes }}"> | ||
{% include site-card.html | ||
subheading="Design assets" | ||
heading="USWDS for designers" | ||
body="The official USWDS design kits in Sketch and Figma." | ||
linkText=uswds_design_kit_link_text | ||
linkUrl="https://www.design_kit.com/community/file/1440921849343185329" | ||
subheadingElement="span" | ||
linkClasses="padding-x-105" | ||
%} | ||
</li> | ||
</ul> | ||
</div> |
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 could not use site-card-list.html here because I needed to pull site data for this card content. I could not do this in the front matter. If you see a better way, let me know!
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.
Confirming it's not possible to pass data into front matter in Jekyl without plugins or potentially hacky work arounds. This is something I've struggled with in the past.
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 see includes that use site data.
{% if include.category %} | ||
<p class="post-category site-subheading"> | ||
{{ post.tags[0] }} | ||
</p> | ||
{% endif %} |
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
On the main what's new page, I removed the tag eyebrow to conserve vertical real estate. I added it back in on the "All news and updates" page. Flag if you would like this changed.
{% if include.date %} | ||
{% include post-meta.html %} | ||
{% endif %} |
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
On the main what's new page, I removed the date to conserve vertical real estate. I added it back in on the "All news and updates" page. Flag if you would like this changed.
@mejiaj @mahoneycm Tagging you both first for review to review the merge strategy outlined in the PR description and confirm that the approach makes sense. Additionally, I am interested in getting initial review of the code approach to identify if there is any major rework needed. I expect that there might be some content and design tweaks, but hoping the layout is nice and solid. Let me know if you have any questions! |
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.
A couple questions and an update to a link!
Overall, looking and working great 👍
pages/whats-new/overview.md
Outdated
<div class="bg-gray-5 padding-3 margin-top-4"> | ||
<h2 class="whats-new__heading margin-top-0" id="latest-releases">Latest releases</h2> | ||
<ul class="usa-card-group whats-new-card-group"> | ||
<li class="{{ product_grid_list_classes }}"> | ||
{% include site-card.html | ||
subheading="Design system" | ||
heading="USWDS" | ||
body="The official U.S. design system, enabling the government to build fast, accessible, mobile-friendly websites." | ||
linkText=uswds_link_text | ||
linkUrl="https://github.com/uswds/uswds/releases" | ||
subheadingElement="span" | ||
linkClasses="padding-x-105" | ||
%} | ||
</li> | ||
<li class="{{ product_grid_list_classes }}"> | ||
{% include site-card.html | ||
subheading="Compilation tool" | ||
heading="USWDS Compile" | ||
body="A tool that makes it easy to customize and compile USWDS Sass into browser-readable CSS." | ||
linkText=uswds_compile_link_text | ||
linkUrl="https://github.com/uswds/uswds-compile/releases" | ||
subheadingElement="span" | ||
linkClasses="padding-x-105" | ||
%} | ||
</li> | ||
<li class="{{ product_grid_list_classes }}"> | ||
{% include site-card.html | ||
subheading="Design assets" | ||
heading="USWDS for designers" | ||
body="The official USWDS design kits in Sketch and Figma." | ||
linkText=uswds_design_kit_link_text | ||
linkUrl="https://www.design_kit.com/community/file/1440921849343185329" | ||
subheadingElement="span" | ||
linkClasses="padding-x-105" | ||
%} | ||
</li> | ||
</ul> | ||
</div> |
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.
Confirming it's not possible to pass data into front matter in Jekyl without plugins or potentially hacky work arounds. This is something I've struggled with in the past.
- title: Page views | ||
source: Pageviews | ||
lead: | | ||
Stay up to date on the latest U.S. Web Design System (USWDS) product launches, learn how to use the design system, and dive deeper into our monthly call topics. |
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.
thought: This is tipping into fine pen review but is learn how to use the design system
appropriate here?
The links here seem less "how to" than other guidance pages on the site. Just a thought!
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.
@finekatie @ceperkins Sending this comment to you for input.
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.
@mahoneycm to confirm, this isn't a blocker right?
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'd say it's not meant literally as a "how to" page, but people can find their way from here to learn how to use the design system. @ceperkins would you agree?
pages/whats-new/overview.md
Outdated
{% assign monthly_call = site.data.monthly-calls.videos[0] %} | ||
{% capture monthly_call_heading %} {{ monthly_call.date }}: {{monthly_call.title}} {%- endcapture %} | ||
{% capture monthly_call_image %} {{site.baseurl }}/img/uswds-logo/4c-lg-on-black.svg {%- endcapture %} | ||
{% capture monthly_call_url %} {{ site.baseurl }}/about/monthly-calls {%- endcapture %} |
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.
Praise: Very cool to see this working! Having this card be dynamic with the latest monthly calls is a great addition!
Co-authored-by: Charlie Mahoney <[email protected]>
@finekatie and @ceperkins tagging you for review on the page copy and layout. Note that the changelog pieces from the prototype will be coming in a follow-on PR (but will all be released at the same time). Let me know if you have questions! |
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.
Left comment about link to Figma releases page link.
heading="USWDS for designers" | ||
body="The official USWDS design kits in Sketch and Figma." | ||
linkText=uswds_design_kit_link_text | ||
linkUrl="https://github.com/uswds/uswds-for-designers/releases" |
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.
My only issue with linking here is that the text says "USWDS design kits in Sketch and Figma" - but on the releases page I don't see anything about Sketch. That would confuse me if I was looking for the design kit for Sketch. What can we do?
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.
That's a good note. Wondering if we just need to add a line to the release notes to signal that Sketch still exists in the package, there just aren't any changes. Alternatively, we could consider shifting the card copy to be more focused on Figma. Curious to hear thoughts.
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.
Yeah this is a Dan/Matt question. Not a blocker though.
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 love the new look.
I've added a small comment to improve clarity of the new jump links include. This is the only issue I found.
Please take a look at the items marked polish or thought. These aren't required changes, but highlighting because they could have helped increase maintainability.
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.
issue (non-blocking): We already have a jump links component in _includes/accessibility-tests/jump-links.html
.
We should avoid repeating ourselves in the future and create a more flexible jump links component that can be reused. This would allow you to re-use the reduced motion styles and avoid setting them in multiple places, like accessibility-tests.html
.
@@ -0,0 +1,25 @@ | |||
<!-- Turn on smooth scroll animation for anchor links --> |
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.
issue: Can you add a comment with a brief description and where this is used?
Are there any settings related to this component?
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.
Added a comment in 84d5637
_includes/jump-links.html
Outdated
<ul class="site-checklist-jump-links"> | ||
{% for link in page.jump_links %} | ||
<li> | ||
<a href="#{{ link.href | default: link.text | downcase | slugify }}"> |
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.
polish: Is downcase needed here? Slugify in Jekyll docs says it already handles lowercase.
Slugify
Convert a string into a lowercase URL "slug".
<a href="#{{ link.href | default: link.text | downcase | slugify }}"> | |
<a href="#{{ link.href | default: link.text | slugify }}"> |
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.
More polish: You could improve readability by saving this in a variable.
{% for link in page.jump_links %}
{% assign link_href = link.href | default: link.text | slugify %}
<li>
<a href="#{{ link_href }}">
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.
Made these updates in 84d5637
<svg class="usa-icon" aria-hidden="true" role="img"> | ||
<use href="{{ site.baseurl }}/assets/img/sprite.svg#arrow_downward"></use> | ||
</svg> | ||
{{ link.text }} |
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.
thought: If link.text
is always available this is fine, but without checking for it you could run into issues.
For example
If it isn't passed you'll still have a list item, link, and icon but it'll be empty and the href will be broken (unless link.href
exists).
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.
It feels to me like link.text
should be mandatory here. Any objection to leaving it as is?
_includes/post-preview.html
Outdated
|
||
<{{ include.heading | default:"h2" }} class="post-preview__title"> | ||
<{{ include.heading | default:"h2" }} class="post-preview__title {{ include.headingClasses }}"> |
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.
polish: Where's this class needed? What happens when headingClasses
is empty?
I see an empty space in the what's new preview
<!-- about/whats-new/ -->
<h3 class="post-preview__title ">
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.
Added a conditional for include.headingClasses
in 9961107
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.
thought: Something for next time. This component could benefit from using capture
to avoid having so many settings. It also gives flexibility to the author by using the markup that they need.
For example:
<!-- Set in template and passed to include. -->
{% capture card_heading %}
<div>A subheading</div>
<h3>The heading</h3>
{% endcapture %}
<!-- Component re-uses `card_heading` -->
<header class="usa-card__header">
{{ include.card_heading }}
</header>
_includes/site-card.html
Outdated
- heading: String heading for a single card. | ||
- headingElement: The heading HTML element type. | ||
- subheading: String subheading for a single card. | ||
- subheadingElement: The subheading HTML element type. | ||
- body: String body that gets placed in a paragraph tag. | ||
- image: The card image. | ||
- imageAlt: The alt tag for the image. | ||
- linkUrl: String for the link url. | ||
- linkText: String link label. | ||
- linkClasses: Classes for the link element. |
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.
issue: Are all these settings required or are some optional?
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.
Added notes about required vs optional in 5d18a6c
.site-subheading { | ||
color: color("gray-warm-10"); | ||
} | ||
|
||
.usa-card__heading { | ||
line-height: unset; | ||
} | ||
|
||
.usa-card__body, | ||
.usa-card__header, | ||
.usa-card__footer { | ||
@include at-media("tablet") { | ||
width: 100%; | ||
} | ||
|
||
@include at-media("desktop-lg") { | ||
@include grid-col(7); | ||
} | ||
} | ||
|
||
.usa-card__media { | ||
@include u-padding(4); | ||
background-image: url("../img/monthly_call_bg.png"); | ||
background-position-y: center; | ||
background-size: 15rem; | ||
display: flex; | ||
justify-content: center; | ||
|
||
@include at-media("tablet") { | ||
position: relative; | ||
width: 100%; | ||
} | ||
|
||
@include at-media("desktop-lg") { | ||
@include grid-col(5); | ||
@include u-padding(6); | ||
position: absolute; | ||
} | ||
} | ||
|
||
.usa-card__img { | ||
background-color: initial; | ||
max-width: 10rem; | ||
|
||
@include at-media("desktop-lg") { | ||
max-width: 15rem; | ||
} | ||
} | ||
|
||
.usa-card__img img { | ||
object-fit: initial; | ||
} | ||
|
||
.usa-button { | ||
@include u-bg("gold-30v"); | ||
@include u-margin-top(1); | ||
@include u-text("blue-warm-90"); | ||
} | ||
} | ||
|
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.
issue (non-blocking): General partials, like typography and card, would have been better places for this.
One could unintentionally add conflicting global styles.
pages/whats-new/overview.md
Outdated
--> | ||
{% assign monthly_call = site.data.monthly-calls.videos[0] %} | ||
{% capture monthly_call_heading %} {{ monthly_call.date }}: {{monthly_call.title}} {%- endcapture %} | ||
{% capture monthly_call_image %} {{site.baseurl }}/img/uswds-logo/4c-lg-on-black.svg {%- endcapture %} |
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.
polish: Using assign
seems more appropriate for these short links.
{% capture monthly_call_image %} {{site.baseurl }}/img/uswds-logo/4c-lg-on-black.svg {%- endcapture %} | |
{% assign monthly_call_image = site.baseurl | append: '/img/uswds-logo/4c-lg-on-black.svg' %} |
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 captures to assigns in 333d592
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 think the Figma/Sketch copy needs a look by Dan/Matt, and we might want to get their feedback on Charlie's question about "learn how to use the design system" but those are not blockers for me. I approve!
…wds/uswds-site into al-whats-new-layout-update
@mejiaj This is ready for your re-review |
Summary
Enhanced the layout and content on the What's new page to improve organization.
Note
This is a follow up to the prototype work in PR #3094. This PR:
Additionally, this PR builds on the updates found in PR #3064. Because of this, I propose the following merge order:
Related issue
Closes #3113
Preview link
Major changes
Testing and review
General
Devs