-
-
Notifications
You must be signed in to change notification settings - Fork 445
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
Redesign search filtering experience to be consistent with mobile #908
Redesign search filtering experience to be consistent with mobile #908
Conversation
Hi @myreli, thanks for the contribution! 🚀 You're not missing anything regarding the tests, btw. For the React and CSS, there are a few things that need to be adjusted, but looks good overall 🙂👍 |
- animate the bubles on search - decouple the Bubbles styling using utility classes - improve styling of the new search options
Hi @amanharwara! Thank you for the review. I have just updated the PR with fdecd9c, that improves the code and result with yours and @moughxyz's suggestions. Could you check again when you have the time? Here's a summary of the changes:
¹ I tried to keep the new utility classes next to the similar existing. However I was unsure when choosing |
|
585f710
to
1055928
Compare
c991296
to
1aa5695
Compare
Hey @amanharwara! Thank you again. |
Looks good! One small issue with this is that since this is using justify-between, if the user increases the size of the notes column, the bubbles would be spread too far apart. I would suggest using regular margins or the |
Got it @amanharwara. I increased gap and justified on center. |
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.
Looks good!
- increase animation timing to match mobile - properly center cancel button - only show cancel on text input - prevent search options from disappearing when clicking with no text
I was giving this a go locally and I think this might actually end up being a bit confusing, as I found out myself. Having just "Protected" conveys that you are filtering based on whether the note is protected or not (like the Archived/Trashed options), instead of conveying that the user will be toggling whether to search contents of protected files or not. It might be better to have the whole "Include Protected Contents" label, which might make sizing tricky, but will be better in terms of clarity. |
Hi! I hope you are doing well today. 😁
This PR provides a redesign for the filtering options. The goal is to provide a more consistent experience cross platform, bringing the search options as bubbles instead of switches on a modal.
screencast_sn_search_options.mp4
Summary
Redesigned the
SearchOptions
component to create a similar experience to the one we have on mobile when filtering:OnFocus
instead of modalBubble
componentonBlur
effect from Search to ensure a more consistent flow with the new experienceApproach
I inspected the experience provided on mobile and did my best to replicate on web, given the difference in usability.
Probably the biggest difference is that I've renamed
"Include protected [Option]"
to simply"[Option]"
to improve readability and avoid horizontal scroll (when possible). Do you think that could mislead users?Thoughts
NoteView
that contains heavy logic. Is there any other place I should consider for e2e or perhaps am I missing something?Thank you for your review!