-
-
Notifications
You must be signed in to change notification settings - Fork 555
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix featured post and resource focus styling #1239
Conversation
* main: Updating Resources for Accessible360 for consistency. (#1237) use fully qualified name for eleventy in scripts Add links to each author's posts in author blurbs (#1232) Typos, extra words & minor punctuation corrections (#1233) Remove extraneous word on contributing guidelines page (#1231) Correct reference to the build tool (not jekyll) in readme (#1230) Update Create accessible forms image (#1228) Add more book resources (#1227) Add image link clarification (#1226) Adds word-break normal in checklist code for screens below wrist-large Rename Myth: Alternate text can be automated (#1224) Copy tweaks (#1223) Copy tweaks (#1222) Add Myth: Alternate text can be automated (#1221)
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 have left few minor comments.
Also, I see that there is now a _includes/resource.njk
and a _includes/card/resource.njk
. As far as I can see, the card/resource.njk
isn't used anywhere at least in this PR. I see that you replaced post.njk
with card/post.njk
. Is this intentional?
<p aria-hidden="true" class="u-font-size-body u-text-transform-uppercase c-banner"> | ||
<svg aria-hidden="true" focusable="false" class="c-banner__icon" width="22" height="22"> | ||
{% if resourceType == "Featured" %} | ||
<use xlink:href="{{ '/img/icons.svg#featured' | url }}"></use> |
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.
Don't have any strong opinion, but it looks like most of xlinks are small case, hyphenated versions of resourceType. Maybe we can use that? Or if we want individual, can we put this mapping of resourceType to xlinks somewhere in data and then just loop through that here instead of using so many if-elif blocks?
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, I'm on the same page. I'd like to keep this scoped and address that in a followup PR, if that's okay with you.
Thanks for the feedback, @SaptakS.
Yes. I've removed Back to you for 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.
Looks good to me!! Waiting for the CI to pass.
This PR addresses #1220. It refactors the featured post and resource components by splitting the concerns up into 3 distinct pieces:
While there is a little DRY code between the featured post and resource partials, I think it will be easier to maintain. The previous partial tried to do too much and be too clever about it, which led to causing issues with the first attempt at a fix.
These videos demonstrate the updated fixes:
featured-post.mov
featured-resource.mov