-
Notifications
You must be signed in to change notification settings - Fork 1
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
GB3-1715: arrow navigation in search #69
GB3-1715: arrow navigation in search #69
Conversation
|
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 Till, sorry I couldn't finish the review yet. But from what I saw yet I cannot approve this requests. There are currently some show stoppers in the frontend.
As for the code: I'll have a look into that probably on Monday. Maybe we'll find some time when you can give me an overview of the changes and your thoughts.
src/app/start-page/components/start-page-search/start-page-search.component.html
Outdated
Show resolved
Hide resolved
src/app/start-page/components/start-page-search/start-page-search.component.html
Outdated
Show resolved
Hide resolved
src/app/start-page/components/start-page-search/start-page-search.component.html
Outdated
Show resolved
Hide resolved
src/app/start-page/components/start-page-search/start-page-search.component.html
Outdated
Show resolved
Hide resolved
src/app/start-page/components/start-page-search/start-page-search.component.html
Outdated
Show resolved
Hide resolved
src/app/start-page/components/start-page-search/start-page-search.component.html
Outdated
Show resolved
Hide resolved
38de742
to
f6f68be
Compare
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'm happy with the results but what did it cost? 😅
Maybe let @Tugark have a second look at the base-search-container.component.ts
.
src/app/shared/components/data-catalogue-overview-item/overview-search-result-item.component.ts
Show resolved
Hide resolved
src/app/shared/components/search/base-search-container/base-search-container.component.ts
Outdated
Show resolved
Hide resolved
src/app/start-page/components/start-page-search/start-page-search.component.ts
Outdated
Show resolved
Hide resolved
src/app/start-page/components/start-page-search/start-page-search.component.ts
Outdated
Show resolved
Hide resolved
src/app/start-page/components/start-page-search/start-page-search.component.ts
Show resolved
Hide resolved
src/app/shared/components/search/base-search-container/base-search-container.component.ts
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.
I had a look at base-search-container
as @wendlans suggested.
I see that there is a lot of black magic going on, so have my approval 👍
On a more serious note: Can you have a look at the duplication warnings by Sonar and if possible, reduce/fix them? If not, head over to SonarCloud and set their status (e.g. false positive, or acknowledged-WONTFIX). If you can't log in, you maybe don't have rights, but I think we could provide them to you. That way, you would not have to bypass rules.
src/app/shared/components/search/base-search-container/base-search-container.component.ts
Outdated
Show resolved
Hide resolved
@Tugark I looked at the duplications and it's just the |
…tor result map to display single result
… reached with tab
…rom outside the search container or from within the search input field. Delegate responsibility to handle temp maps to directive
a4cb764
to
bb7cf51
Compare
|
This PR adds new directives that allow keyboardnavigation through search result on the map page and the start page.
One directive handles all the keyboard events, the highlighting of the selected result etc.
The other one is just there to mark search results so they can be reached via arrow keys. This are passed via
ViewChildren and VieChild
to the respective container,For this, some adaptations were made to the results:
They are now fully clickable, not just the icons.
The tab navigation has been romved from many elements in order not to conflict with the arrow navigation.
Testing will be added asap