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

Include colors and theme when sharing timetable #3467

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

Conversation

leslieyip02
Copy link
Contributor

@leslieyip02 leslieyip02 commented Jul 27, 2023

Context

Resolves #3423 by allowing module colors and theme to be shared via URL.

Implementation

Module colorMapping and themeId are passed into the query parameters.

e.g.

modules = {
   CS2104: { Lecture: '1', Tutorial: '2' },
   CS2107: { Lecture: '1', Tutorial: '8' },
}

colors = {
   CS2104: 3,
   CS2107: 4,
}

themeId = 'monokai'

=> CS2104=LEC:1,TUT:2,COL:3&CS2107=LEC:1,TUT:8,COL:4&THEME=monokai
demo.mp4

@vercel
Copy link

vercel bot commented Jul 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nusmods-export ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 8, 2023 3:48pm
nusmods-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 8, 2023 3:48pm

@vercel
Copy link

vercel bot commented Jul 27, 2023

@leslieyip02 is attempting to deploy a commit to a Personal Account owned by @nusmodifications on Vercel.

@nusmodifications first needs to authorize it.

@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Patch coverage: 63.15% and project coverage change: +0.04% 🎉

Comparison is base (b7c679e) 53.29% compared to head (484b1bc) 53.34%.
Report is 37 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3467      +/-   ##
==========================================
+ Coverage   53.29%   53.34%   +0.04%     
==========================================
  Files         271      271              
  Lines        5850     5892      +42     
  Branches     1395     1406      +11     
==========================================
+ Hits         3118     3143      +25     
- Misses       2732     2749      +17     
Files Changed Coverage Δ
website/src/actions/export.ts 11.11% <0.00%> (ø)
website/src/bootstrapping/matomo.ts 24.24% <0.00%> (ø)
website/src/entry/export/TimetableOnly.tsx 0.00% <0.00%> (ø)
website/src/reducers/timetables.ts 85.39% <ø> (ø)
website/src/serverless/handler.ts 0.00% <0.00%> (ø)
website/src/types/views.ts 100.00% <ø> (ø)
website/src/utils/error.ts 15.38% <0.00%> (ø)
website/src/utils/ical.ts 98.55% <ø> (ø)
...ebsite/src/views/components/SearchkitSearchBox.tsx 0.00% <0.00%> (ø)
...c/views/components/filters/DropdownListFilters.tsx 41.37% <0.00%> (ø)
... and 45 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@leslieyip02 leslieyip02 changed the title Share timetable export theme Include colors and theme when sharing timetable Jul 27, 2023
website/src/utils/timetables.ts Outdated Show resolved Hide resolved
website/src/utils/timetables.ts Outdated Show resolved Hide resolved
}

// Extracts module configs from query string
export function deserializeTimetable(serialized: string): SemTimetableConfig {
Copy link
Member

Choose a reason for hiding this comment

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

It's best to rewrite this so that it parses from query string, then delegate to objects to extract individual parts instead of having each functions re-parse the query string.

website/src/views/timetable/ShareTimetable.tsx Outdated Show resolved Hide resolved
Comment on lines 165 to 174
if (semester && importedColors) {
Object.entries(importedColors).forEach((colorMapping: [string, ColorIndex]) => {
const [moduleCode, colorIndex] = colorMapping;
dispatch(selectModuleColor(semester, moduleCode, colorIndex));
});
}
const importedThemeId = deserializeTimetableThemeId(location.search);
if (semester && importedThemeId) {
dispatch(selectTheme(importedThemeId));
}
Copy link
Member

Choose a reason for hiding this comment

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

Note that imported timetable is temporary - the user has to confirm before the existing timetable is overwritten. This makes the imported module colour and theme take effect immediately when opening the URL, which is inconsistent with timetable behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

demo-v2.mp4

Is this better?

website/src/utils/timetables.ts Outdated Show resolved Hide resolved
Copy link
Member

@ZhangYiJiang ZhangYiJiang left a comment

Choose a reason for hiding this comment

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

Sorry the previous review submitted early. The serialization and deserialization looks good, but the changes cannot be applied immediately when the import URL is opened if we want to match existing import behavior

website/src/utils/timetables.test.ts Outdated Show resolved Hide resolved
serializedModule.split(QUERY_PARAM_SEP).forEach((param) => {
const [paramName, paramValue] = param.split(QUERY_PARAM_VALUE_SEP);
if (paramName === QUERY_PARAM_ABBREV[COLOR_PARAM]) {
colorMapping[moduleCode] = parseInt(paramValue, 10);
Copy link
Member

Choose a reason for hiding this comment

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

Since colours are bound to a sub-param inside each module code, this is better extracted as part of deserializeTimetable instead

website/src/utils/timetables.test.ts Outdated Show resolved Hide resolved
@leslieyip02
Copy link
Contributor Author

leslieyip02 commented Jul 31, 2023

I wanted to implement undoing colors and theme imports, but it seems that the "UNDO" button in the notifications isn't working. I have opened a new issue for that: #3475.

@leslieyip02
Copy link
Contributor Author

I wanted to implement undoing colors and theme imports, but it seems that the "UNDO" button in the notifications isn't working. I have opened a new issue for that: #3475.

The issue has been resolved. Imported colors can be undone, but the theme does not become undone. I'm not sure why the theme is not stored in the history. I'm guessing it has something to do with this:

history.push(timetablePage(semester)); // TODO: Check that this works

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.

Colours are not preserved when sharing timetables
2 participants