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

feat: add core curriculum chips to injected popup #372

Merged

Conversation

tildezero
Copy link
Member

@tildezero tildezero commented Oct 22, 2024

resolves #337

image:
image

Huly®: UTRP-356


This change is Reviewable

Copy link
Member

@IsaDavRod IsaDavRod left a comment

Choose a reason for hiding this comment

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

Doesn't look like it works for courses with the Mathematics or Visual and Performing Arts core. Also, can you see if the text can be more centered somehow (see second pic to see what I mean)? It looks a bit off (looks like N1 is aligned left or something)

image image

@DereC4 DereC4 added the feature label Oct 22, 2024
@DereC4 DereC4 added this to the v2.1.0 milestone Oct 22, 2024
@DereC4 DereC4 requested review from IsaDavRod and DereC4 October 22, 2024 05:51
Copy link
Member

@DereC4 DereC4 left a comment

Choose a reason for hiding this comment

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

not me literally downloading chrome in order to test this branch 😭😭

for ID chip

image

english composition chip

image

humanities chip

image

gov chip

image

history chip

image

SB chip

image

math chip

image

n1 chip

image

n2 chip

image

VAPA chip

image

Copy link
Member

@IsaDavRod IsaDavRod left a comment

Choose a reason for hiding this comment

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

Two things need fixing:
1: The gap between flag chips and core chips is too big (should be the same value as the gap between two flag chips)
2: It appears that the core chips looks like it has a bigger gap with the course instructor name whereas flag chips don't have an unusually bigger gap.

Also, not sure why, but I had to manually re-add my saved calendar courses from the course schedule to get the flag chip to appear. If the extension were to update, peeps who have an existing schedule wouldn't be able to see core chips unless they re-added the course from the course schedule page.

Two flags (correct gaps)
image

One core (w usually large gap w the instructor name)
image

One flag, one core (w usually large gap between each other)
image

@Samathingamajig
Copy link
Collaborator

Also, not sure why, but I had to manually re-add my saved calendar courses from the course schedule to get the flag chip to appear. If the extension were to update, peeps who have an existing schedule wouldn't be able to see core chips unless they re-added the course from the course schedule page.

that is acceptable behavior for now. In the future, we need a way to rescrape all the data when the extension updates (if we changed something about the Course schema or how data is scrapes), but that is complicated and out of scope for this PR. If you would like, we could block this PR on those other changes, but I don't think that's necessary.

@tildezero
Copy link
Member Author

tildezero commented Oct 22, 2024

image

core chip only ^

image

flag + core ^

Copy link
Collaborator

@Samathingamajig Samathingamajig left a comment

Choose a reason for hiding this comment

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

looking good, sorry for the longer review process but this change is significantly larger than most of the other "simple" tasks by its nature

just a few simplification comments to ensure consistent code quality

src/views/components/common/Chip.tsx Outdated Show resolved Hide resolved
src/views/components/common/Chip.tsx Outdated Show resolved Hide resolved
src/views/components/common/Chip.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@Samathingamajig Samathingamajig left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@IsaDavRod IsaDavRod left a comment

Choose a reason for hiding this comment

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

LGTM yayy

@Samathingamajig Samathingamajig merged commit 6f1afc5 into Longhorn-Developers:main Oct 22, 2024
14 checks passed
Copy link

sentry-io bot commented Oct 23, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ TypeError: Cannot destructure property 'courseName' of 'kt' as it is null. HeadingAndActions(../../src/views/components/in... View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Similar to the course flags chip, add a "Core Curriculum" chip, w/ tooltip.
4 participants