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.

Sign up for GitHub

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

Jump to bottom

Show latest term if term indicated by URL is invalid #112

Merged
merged 10 commits into from
Jan 8, 2025
Merged

Show latest term if term indicated by URL is invalid #112

merged 10 commits into from
Jan 8, 2025

Conversation

snowmonkey18
Copy link
Contributor 

    // TODO: - make this nicer, without the try/catch
    //       - actually redirect to a valid URL
    //       - redirect 's'->latestSpring, 'f'->latestFall, etc.
    //       - have a visual cue if the term in the url is invalid

@snowmonkey18 snowmonkey18 mentioned this pull request Dec 31, 2024
Merged
@dtemkin1 dtemkin1 requested review from psvenk and dtemkin1 December 31, 2024 04:50
dtemkin1
dtemkin1 approved these changes Jan 2, 2025
View reviewed changes
Copy link
Collaborator

@dtemkin1 dtemkin1 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 to me! @psvenk any comments before merging?

src/components/App.tsx Outdated Show resolved Hide resolved
src/components/App.tsx Outdated Show resolved Hide resolved
psvenk added 5 commits January 6, 2025 17:30
@psvenk
Use async/await syntax
2a70ee6 
This avoids the nesting indentation that comes from multiple sequential
requests.
@psvenk
Redirect instead of silently loading a valid term
83726da 
This resolves one of the TODO comments, but it is useful since it means
that the URL in the address bar matches what is shown on the page.
@psvenk
Explicitly check against urlNames
2157ebf 
To avoid try/catch.
@psvenk
Redirect to the "closest" urlName, not necc latest
6f60470 
IAP or summer of a year where those terms were not scraped separately
should redirect silently to spring or fall of that year, and otherwise
we try to redirect (eventually with some sort of notice to the user) to
the same semester of the current year.
@psvenk
Add warning after redirecting from invalid term
f8c77a6
psvenk
psvenk approved these changes Jan 7, 2025
View reviewed changes
Copy link
Member

@psvenk psvenk left a comment

Choose a reason for hiding this comment

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

Thanks for the PR; I've gone ahead and resolved all of the TODOs. Let me know if you have any comments pertaining to my commits; otherwise, I think this should be ready to merge. (I don't know why CI isn't running.)

@dtemkin1
Copy link
Collaborator

dtemkin1 commented Jan 7, 2025 •
edited
Loading

(I don't know why CI isn't running.)

Might have to rebase on main? But it's probably fine (edit: nvmd I just had to approve the CI run...)

@dtemkin1 dtemkin1 merged commit 388bdb2 into sipb:main Jan 8, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtemkin1 dtemkin1 dtemkin1 approved these changes

@psvenk psvenk psvenk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@snowmonkey18 @dtemkin1 @psvenk