Skip to content
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

Prerequisite unmet popup in search results #458

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

timobraz
Copy link
Contributor

@timobraz timobraz commented Mar 6, 2024

Description

When searching for a course, an indicator may show up in the search bar if you haven't met prerequisites from your current roadmap.

Screenshots

Screen.Recording.2024-03-06.at.2.17.37.PM.mov

Steps to verify/test this change:

  • Verify changes work as expected on staging instance

Final Checks:

  • Verify successful deployment

(optional)

  • Write tests
  • Write documentation

Issues

Closes #451

@timobraz timobraz added frontend Front End tasks RFC Request for comment labels Mar 6, 2024
@timobraz timobraz requested a review from js0mmer March 6, 2024 22:24
@Awesome-E
Copy link
Member

I think the icon itself is not very easy to tell what it means. Why not just make it consistent with what happens if you actually put it on the roadmap, that is, use the warning sign icon (at least for now)?

@Awesome-E
Copy link
Member

I also think in that for the roadmap in general, the warning and info can probably be combined into one icon, though that affects more areas than you're addressing in this PR

@timobraz timobraz self-assigned this Mar 7, 2024
Copy link
Member

@js0mmer js0mmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the warning icon should be moved to the bottom left so it is consistent with the roadmap. Additionally, top right would conflict with #462.
Why not reuse the existing warning popover/logic?

@timobraz
Copy link
Contributor Author

I think the warning icon should be moved to the bottom left so it is consistent with the roadmap. Additionally, top right would conflict with #462. Why not reuse the existing warning popover/logic?

The current popover logic relies on finding invalid courses that are already in the roadmap by going through each course and creating an array of objects that have location and required fields.

This will probably have to be refactored in a PR of a different scope because the current way the roadmap detects unmatched prerequisites isn't reusable to SearchHitItem.

Copy link
Member

@js0mmer js0mmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. My one concern though is that the logic for this is implemented in SearchHitContainer component which is also reused for the course/professor catalog pages. It doesn't seem to be cause any errors though.

@js0mmer js0mmer self-requested a review May 8, 2024 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Front End tasks RFC Request for comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add prerequisite met indicator within search module
3 participants