-
Notifications
You must be signed in to change notification settings - Fork 14
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
Roadmap Search Redesign (Part 1) #514
Conversation
(preliminary)
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 grabbing cursor only shows when you first start the drag and are still hovering over the original spot which bothers me. I think the way around would have to be adding/removing the cursor: grabbing
style to the document body within the onStart and onEnd listeners for dragging.
Re-requesting review. I applied the cursor style via CSS instead – still seems like it doesn't work when dragged over a scroll bar, but I don't think that's an issue we have to worry about too much. |
Not sure if @js0mmer will review since he's graduated, so asking @timobraz for code review and @CadenLee2 for staging check |
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.
Oh, didn't see that you re-requested. LGTM
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.
Tested staging instance in Firefox on desktop and on Android
- Quarter widths look good regardless of the number of quarters included, on a wide screen there is a lot of empty space but that's better than over-stretching them so this makes sense:
- Saved courses show up properly when there's nothing in the search bar
- When no courses are saved, there is a simplistic placeholder message under the search bar without the anteater illustration, as opposed to the current "Start typing in the search bar" prompt with the illustration (is this intended? It feels less polished)
- When the search has no results, nothing shows up at all, as opposed to the current "Sorry, we couldn't find any results for that search!" message with the anteater illustration (see the staging screenshot below). I think this is a problem, since there should at least be something to show that there aren't any matching courses?
Old:
New:
- The new mobile add course menu works (closing, opening, adding courses, viewing saved courses when there is no search bar input), but has the same behavior as the above when there are no search results/when there are no saved courses
- Styles all look good
With the placeholder messages, I do prefer what was there before--was there a reason for the change, or is it an issue? If it's the latter, it shouldn't be too difficult to fix. Otherwise, I can approve this as well.
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.
Placeholder improvement looks good to me
Description
SortableJS instead of React Beautiful DND
This library makes the animations more smooth when being dragged (cloned) from the search bar and when being dropped (inserted) into a quarter in the user's roadmap.
Previously, it used to move the dragged element into the quarter, then have a sudden transition in it's shape as it re-rendered the component. Now, there's a placeholder component when dragging so there's no layout change.
Compare https://peterportal.org/roadmap vs https://staging-514.peterportal.org/roadmap
Search Layout and Desktop Year Layout
Before (left) and after (right):
Mobile "Add Course" Menu
Before:
After:
Steps to verify/test this change:
Issues
Didn't find any