-
Notifications
You must be signed in to change notification settings - Fork 481
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
New Search Modal UI #2202
New Search Modal UI #2202
Conversation
- Fixed the collapse/expand issue when clicked repeatedly - Remove extra space from the navbar header icons
I think the only thing to do now would be,
|
Awesome! A couple of thoughts:
Also cc @fredrikekre -- what are your thoughts on this UX in general (i.e. replacing a separate search page with a modal)? The other two fixes -- could you open separate PRs for those please? |
Great work! From the invitation on Slack to give feedback, I took the demo site for a test drive, and these are the things I noticed (using Firefox on an M1 Macbook):
Please don't take these points as criticism but rather as suggestions for further improvements. As stated above, overall great work! |
Grabbag of unfiltered thoughts trying it out. When I click down on the text box the browser shows some auto complete entries until I do mouse down and then they disappear and the text box shows up. It also somehow feels quite surprising to have a input text box spawn a new floating window. These files like they should be code quoted like they are in the manual itself. The scroll bar is very narrow for me and hard to click on with the mouse. If I am on the right half of the scroll bar it seems to not reach and I have to position the mouse on the left-most part of it There is no need for the ... here? ctime is the full name of the function? Is there any value in the short cut command for search being shown here? To me it makes it feels like it makes sense to use that short cut at this current moment The recent searches list doesn't seem to ever show anything |
We had discussion over this before. #2027
Do you mean the gap between the search results?
While, I do not understand what you exactly mean but by hunch do you want to display a code snippet or something like that? If so, that would require some thinking(yeah, out of scope for this PR 😅)
We have opted to go with, title, page link and the category as for now. @mortenpi Let me know what are your thoughts on this?
Actually, this was my initial thought but I scraped it because as per the current UI the filters changed too much(every 2 or 3 keystrokes) which can sometimes be jarring for someone, Hence I opted out from it.
This is on purpose, because if I try to display the full link as it is in
This might be due to change in search engine. #2172
We can add features like bookmarks and search history, essentially storing it in the local storage of the browser but local storage has character limitations which vary based on devices. |
Look forward to future PRs, this might be removed entirely and replaced with a icon or input tag in the top nav.
@mortenpi If we were to do this, is there some way to know if some title is code quoted or not?
Will be fixed.
Will look into that.
No, those are for informative purposes only.
This is just a placeholder. There is no feature for storing search history as of now. 😓 |
- Added modal close button - Reduced some vertical spacing - Increased scrollbar width
Done
Added the "X", but it is not full-screen.
Suggestions are appreciated... 😄
Done |
I think this is a fair point. It would be good to keep that information somewhere in the new UI.
I think this is also a good point. In the current implementation, you can use the That said, I just noticed it's not fully working -- if you update the search query while on the search site, it doesn't update the URL. All in all, I think this would be nice to have, but is probably not critical. And since the current implementation is only half working, then removing this is maybe not even a regression. It would also be good to know if there are any other modal-ish searches that have a way of linking to searches. I don't think we want to implement any UI here specifically for bookmarking though -- that should be left to the browsers etc.
I think replacing the search input box should be done as part of this PR, since the input box doesn't really make sense if we have the modal.
Let's gray out the irrelevant filters maybe? That would be less jarring than DOM elements hopping in and out of existence.
I think we have to look at the class? Functions and types would be code, sections and such would be text. It's also possible to have code in section titles, but let's ignore that for now.
There's a bug here too -- note the
Of course, Documenter also generates pretty crazy ids/fragments sometimes. But I think it should work as is, and point to |
Yes. There is a lot of vertical whitespace that IMHO would be put to better use by being able to view more results at once (i.e., by reducing the vertical whitespace).
For example, Read the Docs provides a small glimpse at the content of the search result, such that one can more easily decide which of the results is likely to be most useful to the user:
The displayed part of the link, i.e., the
No, don't store anything in the browser. But maybe you can do something like Read the Docs does: When you type in a search term, e.g., "search" (see picture above), they also automatically update the URL: This allows one to close the browser tab and reopen it, and it will be exactly at the same place again. Also, it allows me to copy the URL and share it with someone else (like so) and they will be able to access the same search results. |
Yes, e.g., Read the Docs.
I agree and this is not what I had in mind. I hope in my answer above it becomes clearer. |
Hetarth's implementation does show context in some cases now: Although it does looks like for the Julia manual a lot of them are missing somehow. @Hetarth02 any ideas why? In some cases we're probably missing the context in the index. E.g. if we're matching just the title or function name, we should probably show like the first bit of the section or docstring. But I don't think we have that information currently in the index. That should be looked at in a follow-up PR though -- let's not change the index here. |
Ah, cool. Yeah, I think we can take inspiration from that, and just update a query parameter (either in this PR, or a follow-up). Side note: RTD also uses |
- Made the result title bold to better differentiate - Titles for results in function, module, etc. will be in monospace - Added section titles at the end of url - Replaced "No recent searches!" placeholder text - The sidebar input box has been replaced with a div - Fixed a bug where some URLs with special characters didn't generated fully - Updated comments
Edit: 2023-08-20
This should be working, I have tested it locally but it would be helpful if someone can verify this with different urls. |
After you have already searched for something and navigated to the respective documentation, if you search for something else that falls in the same section, the search modal doesn't go away. The documentation itself apparently navigates properly within the same section, but the search modal still stays on the screen. |
Good catch! I can also replicate this. cc @Hetarth02 -- this would be good to get fixed. |
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.
Nice! A few more comments, now that I'm looking more closely at the code:
- Is there any reason to have
shortcut.js
separate fromsearch.js
? Although, maybe the files are getting big enough that it does make sense to split them up. - I think we also still need to remove the generation of
search/index.html
? I didn't see that change in the Julia code?
// Helpers for search modal | ||
.w-100 { | ||
width: 100%; | ||
} | ||
|
||
.gap-2 { | ||
gap: 0.5rem; | ||
} | ||
|
||
.gap-4 { | ||
gap: 1rem; | ||
} | ||
|
||
.gap-8 { | ||
gap: 2rem; | ||
} |
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.
We can leave this as is, but in general I am not a fan of these kinds of helpers -- essentially you're using HTML to do the styling again.
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 find these kinds of helper more useful, maybe it has to do something with me using bootstrap normally...
Here's an updated Julia manual demo: https://mortenpi.eu/juliadocs-demos/2023-08-21-search-demo/ |
- Fix modal not closing when url on same page - Changes as per feeback
If possible, let us keep this as it is for the moment.
Done |
Done! |
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.
LGTM! In my opinion we can merge this. Just needs the CSS files to be updated.
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.
Awesome! Thanks a lot @Hetarth02 for iterating on this! And thanks everyone for the feedback!
Squashed commit of the following: commit da6d4f8 Author: Hetarth Shah <[email protected]> Date: Tue Jan 23 23:52:14 2024 +0530 - Added div at end of file to store metadata - Added metadata.js commit e250ebb Merge: 85828de aac2ea9 Author: Hetarth Shah <[email protected]> Date: Tue Jan 23 23:08:14 2024 +0530 Merge branch 'master' of https://github.com/Hetarth02/Documenter.jl commit 85828de Merge: b2feabd c5a89ab Author: Hetarth Shah <[email protected]> Date: Sun Dec 10 13:20:11 2023 +0530 Merge branch 'JuliaDocs:master' into master commit b2feabd Merge: be00b02 28c5186 Author: Hetarth Shah <[email protected]> Date: Mon Nov 13 16:35:25 2023 +0530 Merge branch 'JuliaDocs:master' into master commit be00b02 Merge: 60ca700 7f7c4bd Author: Hetarth Shah <[email protected]> Date: Mon Nov 13 09:05:23 2023 +0530 Merge branch 'JuliaDocs:master' into master commit 60ca700 Merge: 24d8a5f efb3ab4 Author: Hetarth Shah <[email protected]> Date: Fri Nov 10 16:55:57 2023 +0530 Merge branch 'JuliaDocs:master' into master commit 24d8a5f Merge: 149bdd0 c6abf9d Author: Hetarth Shah <[email protected]> Date: Sat Nov 4 08:01:50 2023 +0530 Merge branch 'JuliaDocs:master' into master commit 149bdd0 Merge: 984ef8c a86549b Author: Hetarth Shah <[email protected]> Date: Sun Oct 15 15:40:57 2023 +0530 Merge branch 'master' of https://github.com/Hetarth02/Documenter.jl commit 984ef8c Merge: f32d6de a232dd4 Author: Hetarth Shah <[email protected]> Date: Tue Sep 26 23:00:19 2023 +0530 Merge branch 'master' of https://github.com/Hetarth02/Documenter.jl commit f32d6de Merge: defe107 bb1dbc4 Author: Hetarth Shah <[email protected]> Date: Thu Aug 24 12:23:09 2023 +0530 Merge branch 'master' of https://github.com/Hetarth02/Documenter.jl commit defe107 Author: Hetarth Shah <[email protected]> Date: Thu Aug 24 12:22:22 2023 +0530 New Search Modal UI (#2202) commit bb1dbc4 Merge: 724bd89 e2bd194 Author: Hetarth Shah <[email protected]> Date: Thu Aug 24 11:48:16 2023 +0530 Merge branch 'master' of https://github.com/Hetarth02/Documenter.jl commit 724bd89 Author: Hetarth Shah <[email protected]> Date: Sun Jul 16 19:08:27 2023 +0530 - Corrected typo commit 1fb8e07 Author: Hetarth Shah <[email protected]> Date: Sun Jul 16 19:05:48 2023 +0530 - Updated the duplication issue - Updated the UI for the actual search results commit 34303e0 Merge: b858db5 4643dbf Author: Hetarth Shah <[email protected]> Date: Sun Jul 16 14:00:18 2023 +0530 Merge branch 'master' of https://github.com/Hetarth02/Documenter.jl into Issue-2141 commit b858db5 Author: Hetarth Shah <[email protected]> Date: Thu Jul 6 01:28:29 2023 +0530 - Changed LunrJs to MinisearchJs for client-side search
Edit by @mortenpi: added the TODO list:
Last Edit by @Hetarth02 : 2023-08-20:
search-result-title
somehow (font-weigth: 600
).search-result-title
for code items (like function names and such; anything exceptPage
,Section
) in monospace (but make font a bit smaller, to match what we do in main body).Type something to get started
.Edit by @mortenpi: close #1437, close #2147
Well, the search UI has had a complete overhaul, with a fresh new modal UI s mentioned in Issue #2147 with better context for search results and a filter mechanism to remove those pesky unwanted results.
Here is the full demo,
Recording.2023-08-06.180832_compress.mp4
Search will now look like this,
Light Mode:
Without Filter:
With Filter:
Dark Mode:
Without Filter:
With Filter:
Since these we very small fixes, I did them while we are updating the search UI altogether.- #2215 Removed some extra space between navbar header icons.- #2103: Fixed, docstring collapse/expand icon not changing when clicking rapidly. Now click all you want!