-
Notifications
You must be signed in to change notification settings - Fork 53
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
[GSoC] Quick navigation UI #396
[GSoC] Quick navigation UI #396
Conversation
…tion * Introduced a new component "QuickNavigationModal" that serves as UI for the new searching functionality. * New regex pattern to match symbol titles by fuzzy and string match. * Data store to control the Quick navigation modal from anywhere on the app without having to pass the data through multiple child compoents. This change is under the quickNavigation feature flag.
…rance of a character
…fiaromorales/swift-docc-render into sr/quick-navigation/algorithm
* Refactor of match highlighter for Safari support using `QuickNavigationHighlighter` component * Code quality improvements suggested on pr review
* Quick navigation modal with redesigned UI * Keyboard navigation support
* `QuickNavigationModal` unit tests * `QuickNavigationHighlighter` unit tests * Styling refactor * Minor UI fixes
@marinaaisa ready 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.
Thanks, Sofía!
I started reviewing your PR, I'll continue tomorrow. I leave you some comments in the meantime.
* Implemented tab navigation on matches list * Changes to sorting logic to create more predictable results * Made matching algorithm case insensitive * Code cleanup
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.
Hi Sofía, thanks a lot for this huge PR! It's great work 🙌 I added some comments.
My main concerns is to see how we can do FilterInput more generic without adding specific logic to it. I left comments about it. Also, there is an AX issue with the keyboard tabbing backwards, I left another comment.
Congratulations on the tests, I'll see if we can add more and let you know. But your current progress on testing is very very good :)
* Freezing the index added into the store
* `QuickNavigationBar` component * `QuickNavigationBar` test file
… on the value of `enableNavigator` to prevent trying to fetch data when the archive does not come with an index. Also, the logic to open `QuickNavigationModal ` depends on `enableNavigator`.
…into a utility file to clean up `NavigatorDataProvider`
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.
Hey @sofiaromorales , I left couple of more comments.
I'll continue reviewing next week, hopefully we get to merge it soon, sorry for the delay. It's taking a long time but the progress and feature is big and amazing! Thanks again! :)
color: var(--color-figure-blue); | ||
} | ||
} | ||
&__match-list { | ||
overflow: scroll; |
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.
In Chrome, with an macOS system config of "Show scroll bar... Automatically"
This overflow scroll is creating a jumping effect and also an extra padding in the bottom when there is no results.
overflow-jump.mov
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.
Thanks for catching this, Marina. I fixed the padding overflow by setting the height to 0, but the layout shift is more tricky to fix, scrollbars, when set to show always, reposition the web content causing that jumping effect, overflow-y: overlay
seems to fix it but is not widely supported by all browsers.
https://developer.mozilla.org/en-US/docs/Web/CSS/overflow#browser_compatibility
You can check the implementation here 76dfe47
Just a quick UI detail: Whenever we have the focus on the Quick Navigation's filter, we may want to delete the border line on top of the match list. To achieve this you can add this line of CSS
|
…ser input into a valid string for fuzzy match
`indexData`utils file renamed to `navigatorData` Removed border-top form match list on focus
Thanks a lot for the review and patience @marinaaisa! Do you think it's safe to merge this already? I want to give people the opportunity to test quick navigation in their documentation, this way I can receive feedback and discover new bugs if any, and since it's under a feature flag, it will not affect the current functionality of the project |
@swift-ci test |
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.
Congratulations on your hard work, Sofia, it's ready to be merged :)
Bug/issue #, if applicable: NA
Summary
UI for Quick Navigation feature with keyboard navigation support
Dependencies
NA
Testing
Steps:
quickNavigation.enable
flag to true insidetheme-settings.json
in the swift project theming settings (https://www.swift.org/documentation/docc/customizing-the-appearance-of-your-documentation-pages)/
key or⌘ + shift + o
Enter
to access the symbolDemo: https://sofiaromorales.github.io/SwiftDocC/documentation/swiftdocc
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
npm test
, and it succeeded