-
Notifications
You must be signed in to change notification settings - Fork 538
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
Update navbar design and improve search UI #1084
base: master
Are you sure you want to change the base?
Conversation
🚀 Preview for commit fcc9565 can be found at https://web-php-pr-1084.preview.thephp.foundation |
🚀 Regression report for commit fcc9565 is at https://web-php-regression-report-pr-1084.preview.thephp.foundation |
Suggested reviewers: @morrisonlevi @dragoonis @cmb69 @kamil-tekiela @ramsey @mvriel @pronskiy @Girgias @localheinz Edit: Will check visual tests asap |
I like it. I like the changed color, I like that the text of the buttons is more readable, I like that the PHP logo is now white, and I like that the search results list is simpler. The mobile view looks really good. There are a couple of things I don't like though.
|
Thank you! Overall, this looks like a nice improvement, but I don't like that the search now won't work if JavasScript is not available. |
The popup search results looks very smart. A few thoughts: Accessibility:
Other:
|
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.
The search bar not working any more with JS is a massive issue.
Adjust navbar and search modal element colors to meet WCAG AA contrast ratio. Additionally, match the search button label and input placeholder text to avoid confusion, and hides some SVG images from the accessibility tree.
CSS BEM is used in the PHP 8 Release pages and Special Thanks pages.
Thanks for the quick review and feedback! I've addressed many of your points and would like to discuss a few of them: Progressive enhancement@cmb69 @Girgias @simonrjones Searching on mobile sized screens will still require JS. Given that it's common for mobile users to have JavaScript enabled, I think this is a reasonable trade-off. Some major language require JavaScript for searching (even on desktop): Kotlin, Rust, Swift, MDN, Dart. All Algolia-powered docs as well, and MkDocs. Accessibility and contrastAll elements in the navbar and search modal have been updated to comply with WCAG AA contrast ratios. This includes the primary navigation items, search text, and icons in mobile view. LayoutPage covered, no scroll@kamil-tekiela The popup is inspired by Algolia that used in popular documentation sites (e.g. Composer, PHPStan, Laravel, Vue, React etc). It felt it a bit awkward the first time I saw it, but got used to it eventually. The ideal UI for me would be something like Psalm docs (Material for MkDocs), but it is a bit more complicated to implement. Given the current search UI situation (5 different scrolling elements nested in a thin menu) maybe we could proceed with the Algolia-like modal as a first step and further improve it in the future. Would you consent to this approach? Fade animation@kamil-tekiela I believe this is a bit subjective and a matter of taste, but I can remove it if it's a common opinion. For now I halved the animation duration to 1/10th of a second, is this still a deal breaker? You can test it here (may need to disable cache). Navbar height@kamil-tekiela Forgot to mention but I followed @morrisonlevi's suggestion to avoid the sticky header, so now it doesn't take up any space after scrolling down. Note that the navbar is 64px tall on both desktop and mobile, which seems a common height for navbars, even sticky ones. Other fixes
Unrelated or out of scopeRelease page formatting@kamil-tekiela I think this is now fixed php/phd#156 Search page styling@simonrjones Given that this is the current behavior and it relates to the page content, I think it would be out of scope for this PR. What do you think of discussing this in a separate issue? |
Additionally, increase contrast of navbar items and make some minor adjustments and fixes.
393d3c4
to
2f3da84
Compare
Just pushed some commits addressing both points. @Girgias @simonrjones @kamil-tekiela Changes are deployed to https://php-navbar.lhsazevedo.dev . There is one pending fix to ensure that it will gracefully update to the new local index cache format. I'm planning to push it (and fix CI) by next week |
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.
Some minor nits, but LGTM.
I will let other people more in charge with the Website take over.
js/common.js
Outdated
) { | ||
$("#goto .results ul").append(` | ||
<li> | ||
<a href='/manual/en/${node.id}.php'> |
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 probably should use the current language instead of directing to the English version.
<a href='/manual/en/${node.id}.php'> | |
<a href='/manual/LANG/${node.id}.php'> |
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.
Yup, it probably should. Actually, this code is part of the "global search" feature that triggers with g then a. I wasn’t aware of it until I had to adjust the code to prevent breaking due to changes in the search index structure.
TBH, I believe that we could just remove it and use g a as another shortcut to the new search dialog, but may be out of scope for this PR.
For now I've pushed a commit for the anchor href you pointed and the cache key a few lines before it.
Co-authored-by: Gina Peter Banyard <[email protected]>
Simplified function signatures by avoiding redundant language parameter passing, now using the one available in the outer scope. Also avoids shadowing the language variable.
Additionally, use brand blue as the selected color for search results
4522fe1
to
39bc218
Compare
I think keep header position fixed, but after scrolling resize to small is good |
77ba1cf
to
eb7a39d
Compare
@saundefined can you have another look at this? |
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 like it, thank you!
We can wait a little longer (say, until the end of the week),
and if there are no objections, merge it
Sounds good, no rush. It would be great to have this ready for the 8.4 release though. |
Fixes #502 |
Can the mobile nav sidebar be made to close when tapping outside of it? I instinctively tried to do this when testing on my phone, and it feels annoying that I instead have to tap a small X in the corner. |
@lhsazevedo can you take a look?
Should solve it |
Thanks @saundefined, merged. I noticed some common code between the offcanvas menu and the search modal. Lines 315 to 321 in eb7a39d
In the future, we could consider refactoring this into a helper to keep things DRY. But hey, it's always good to go WET first! |
Hero classes changed after php#1107. Should fix visual tests.
Tip
You can view this proposal live at: https://php-navbar.lhsazevedo.dev/
Intro
Over the years, the PHP webpage received various design proposals, some didn't gain traction due to their drastic nature or departure from PHP's established branding style. However, our community has shown that we can successfully implement design improvements when they're incremental – the PHP 8 release pages, homepage hero and thanks page are great examples of this approach.
Inspired by these successful updates and the the discussions they sparked, I'd like to propose some updates to the navbar design and introduce a new search dialog to enhance a bit the user experience on php.net. These changes are inspired by modern web design principles while respecting PHP's branding and build upon previous community discussions.
By focusing on small enhancements rather than a full redesign, we can improve the site's usability and aesthetics without disrupting its familiar layout and branding.
UI Scope
It's worth noting that while the current client-side search implementation could be considered outdated, replacing it would be beyond the scope of this design improvement. Such a change would require a separate, focused effort. This proposal aims to enhance the user interface while maintaining the existing search functionality, setting the stage for potential future improvements to the search implementation itself.
Enhancements
Modernized navbar design:
New search dialog:
Retained search implementation:
Code improvements
Enhanced reliability
Impact
These changes are designed to be minimally disruptive:
Preview
Please note that the preview site uses an improved search index generated by a companion change I'm proposing to the PHP documentation generator (phd). While both PRs can be merged independently, I've captured the screenshots and recordings using the new index to showcase the full potential of these improvements.
You can find the companion PR for the phd changes here: php/phd#154
Preview Site
You can view this proposal live at: https://php-navbar.lhsazevedo.dev/
Desktop preview
desktop_preview-2024-10-03_22.08.18.mp4
Mobile preview
mobile_preview-2024-10-03_22.10.47.mp4
Comparison
View GIF comparisons (image heavy)
Desktop comparison
Navbar
Search modal dialog
Mobile comparison
Navbar
Navigation
Search
Final thoughts
I believe these improvements will benefit the PHP community while respecting the site's established design principles. I look forward to your feedback and collaboration in refining this proposal.
If you find this proposal valuable or have any concerns, I'd greatly appreciate your feedback. Please consider leaving a 👍 or 👎 to help measure the community opinion.