-
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 - What's new: Update post preview structure and styles #2979
base: main
Are you sure you want to change the base?
Conversation
…ats-new-post-previews
…ats-new-post-previews
_posts/2024-10-19-accessibility-inclusive-user-experiences copy.md
Outdated
Show resolved
Hide resolved
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.
Looks great! The only thing I wonder about is if we should display the date under the title and authors of each post, instead of at the bottom. I'm not sure I have a strong opinion either way, but wonder if there are considerations for screen reader users and zoom magnification users. Would it be better to display the date under the title/authors?
I moved the date above the post summary text. This is ready for re-review. |
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.
LGTM!
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.
looking great! approved.
@mejiaj and @mahoneycm, I believe I have addressed all of your comments. This is ready for re-review. |
Update: I just restored the |
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 LGTM, added some non-blocking comments. I can approve once resolved.
If you feel any don't apply or will be done in future work, feel free to resolve with a comment stating that.
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: Much cleaner template, nice work.
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 was going to say the same thing! Great work here Amy!
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.
question: This is notional content too, 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.
Yes, these posts are all just notional content to demonstrate potential content types and test the new option to add a preview url.
&__content p { | ||
@include u-font("lang", "xs"); | ||
} |
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: This doesn't make much of a difference and could mean one less style to 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.
Fair. I like that it adds a tiny bit more visual hierarchy, but happy to remove it if that is the team's preference. It is not a must-have for me.
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's really a tiny difference of 15.04px → 16px.
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.
LGTM! Great work here Amy!
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 was going to say the same thing! Great work here Amy!
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.
Thinking this looks really good! Approved.
@annepetersen and @thisisdano, this is ready for your review. |
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.
Completely a content item, but I'd love to stop using "Here is" or "Here you can" from the lead of any page. You can usually omit it and still accomplish what the lead needs to do. Tagging @finekatie to maybe include this in voice/tone/guide materials we're intermittently working on.
Not a blocker.
Otherwise, looks good! Would obvs need a changelog and real first post, replacing any notional material. 😅
Hi @annepetersen , I'd recommend taking it out and tweaking so the first sentence reads "Get the latest USWDS news and information here." I've also drafted a bunch of posts (see #2989) for the page and to edit/approve content - some of which are already in the preview. There are three more for November and we can add more with the monthly call content as that's approved. :) |
Update:
Screenshot of styles for different post types: |
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 works! Approved.
Summary
Updated the post previews on what's new page. This PR changes the styling of the post previews and also allows for custom urls in the preview header link via
preview_url
.Example:
Important
This PR includes sample posts to demo content. We need to remove all of the changed files inside theRemoved demo posts in 61aaf20_posts/
directory before merge.Related issue
Closes #2953
Preview link
Problem statement
Content limitations:
Usability concerns:
Accessibility concerns:
<article>
wrapper. This makes sense for post previews, but not the post pages themselves.Solution
Post previews:
Condensed the size of the previews and updated the structure to allow new content types:
preview_url
data key in the pages front matter to set an alternate URL for the header link.Posts
<article>
from the post pageCode
post-preview.html
and pulled out all the "excerpt" conditionals frompost.html
. This was done to improve readability.post.html
to SassTesting and review
Accessibility
Post previews
Posts