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

Shorten timetable share URL #3367

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

Conversation

winphong
Copy link

@winphong winphong commented Dec 3, 2021

Context

This PR fixes #2057 where timetable share url is not shortened

Implementation

Loading spinner while url is being shortened

Display show original URL button when URL is shortened

NOTE: Used a separate url shortener service to mock url shortening behaviour

Display shorten URL button when URL is full

URL shortening errored

Other Information

👋🏻 my first time contributing to a project, implemented the solution based on my personal experience on user experience, open to suggestion if there's a better UX. Tried my best but not too familiar with UI test so do let me know if there's something that I can improve on. Thanks 🙏🏻

@vercel
Copy link

vercel bot commented Dec 3, 2021

@winphong is attempting to deploy a commit to the NUSMods Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Dec 3, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

nusmods-export – ./export

🔍 Inspect: https://vercel.com/nusmodifications/nusmods-export/AVdmcWRd2grXypgL8H8fWGEuVKJe
✅ Preview: https://nusmods-export-git-fork-winphong-url-shortener-nusmodifications.vercel.app

@codecov
Copy link

codecov bot commented Dec 3, 2021

Codecov Report

Merging #3367 (7cb3921) into master (1160b6f) will increase coverage by 0.18%.
The diff coverage is 96.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3367      +/-   ##
==========================================
+ Coverage   53.08%   53.27%   +0.18%     
==========================================
  Files         270      270              
  Lines        5745     5757      +12     
  Branches     1327     1332       +5     
==========================================
+ Hits         3050     3067      +17     
+ Misses       2695     2690       -5     
Impacted Files Coverage Δ
website/src/featureFlags.ts 100.00% <ø> (ø)
website/src/views/timetable/ShareTimetable.tsx 81.42% <96.29%> (+13.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1160b6f...7cb3921. Read the comment docs.

Copy link
Member

@li-kai li-kai 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 great work, and apologies for the delay. The code and tests look good to me. @chrisgzf I'll leave the merging to you? We should also open a task to bring our url shorterner back up.

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

Successfully merging this pull request may close these issues.

Timetable share URL is no longer shortened
2 participants