-
Notifications
You must be signed in to change notification settings - Fork 4
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
add dnd support to search panel, highlight searched rule, UI adjustments #640
Conversation
yuntongf
commented
Apr 9, 2024
- Can now drag a course from search result to plan panel, req panel, dock
- When user clicks search for a specific rule, the rule will be highlighted on the req panel as the search panel displays search results
- Reduce gap between req and search panels
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.
couple of questions
${props => !!props.$isSearched && ` | ||
border-radius: 10px; | ||
box-shadow: 8px 6px 10px 8px #00000026; | ||
`} |
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 think this shadow looks a little weird bc it's so strong. Maybe we could change the color or apply some other effect to the surface of the search condition when clicked? let me know what you think
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 changed this to lighter and less aggressive shadow, similar to the one shown for droppable boxes
Revert "Aug/add semester popup"
const handleCloseSearch = () => { | ||
setQueryString(""); | ||
setSearchPanelOpen(false); | ||
setSearchedRuleId(-1); |
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 think the value should be null instead of -1
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.
Yea makes sense
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 except 1 comment. Just gonna approve but if you could fix that and check ui after #650 is merged that would be amazing