-
Notifications
You must be signed in to change notification settings - Fork 6
New design for wpaccessibility.org #220
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
base: main
Are you sure you want to change the base?
Conversation
|
…18-new-design # Conflicts: # docs/topics/forms/fieldsets.md
joedolson
left a comment
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 some minor comments, but on the whole I think we should just merge this. From a practical standpoint, this is very hard to review - combining a refactor of the docs JS with other changes is something I wouldn't generally do, as it makes the diff much harder to parse.
There are some minor things - like I think the vertical height of the secondary nav items is too tall, and I don't fully agree with the accessible name of the primary logo with the img inside the link, but I think they can be adjusted as follow ups.
The description is in the index and shown in results, but isn't being searched; that'll probably need to be tweaked, as well.
| @@ -1,23 +1,22 @@ | |||
| <div class="search"> | |||
| <form class="search-input-wrap" role="search" action="{{ site.baseurl }}/search/" method="get"> | |||
| <search class="search"> | |||
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.
Do you think this has sufficient compatibility? The search element has only been supported on Safari since version 17, so older Macs & iOS devices may not get the landmark role from the element. Other browsers are on similar timelines, but Safari is unique in that devices that can't be updated to more recent OS also can't update their browsers.
| <img src="{{site.baseurl}}/assets/images/logo.jpg" alt="Accessibility: Advocate Equal Access"> | ||
| <a href="{{ '/' | relative_url }}" class="site-title lh-tight"> | ||
| {{ site.title}}</a> | ||
| <img src="{{site.baseurl}}/assets/images/logo-wa11ypuu.png" alt="WP Accessibility logo, to homepage"> |
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 don't personally like having the accessible name of the link have all this information in it. This becomes "WP Accessibility logo, to home page WP Accessibility Knowledge Base", which seems excessive to me.
|
|
||
| .nav-list-item { | ||
| position: relative; | ||
| min-height: 48px; |
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 this gives too much height on secondary nav items. I'd remove it and use something like padding: 4px 0;
The related issue number: #218 (new design) and #221 (description to search)
Design Jeffrey Lauwers:
https://www.figma.com/design/MTMLnjkAGJvYrZjTeu5ert/WP-Accessibility-Knowledge-Base?node-id=33-1237&t=byeUhJr81gGKjPNv-0
Preview: https://wpaccessibility.org/pr-preview/pr-220.
Note:
The search on mobile is not the best experience ux-wise.
We need to discuss if the drop down on mobile is useful and if we can move the search on. mobile above the menu.