Sort by semester #537

zachary-kent · 2021-10-06T02:59:17Z

Summary

This pull request implements a dropdown menu that allows the user to select whether their semesters are shown in order of newest to oldest or oldest to newest, persisting their choices in the backend. The dropdown menu also allows users to select whether semesters are displayed in compact or default layout, although this choice is not persistent. Implementing these changes required adding an orderByNewest field both to the user-semesters collection in Firebase and to the Vuex store. To minimize reads and writes to the Firebase, changing the ordering preference on the frontend only modifies the orderByNewest field in the Firebase. As a result, the semesters are no longer sorted in the backend; to ensure this does not introduce any breaking changes, when the orderByNewest field is changes, the semesters are resorted in the Vuex store--but not in the Firebase. Nevertheless, their still exists non-negligible latency when changing the ordering preference on the frontend, something to be explored in the future.

Remaining TODOs:

Reduce order preference latency
Fix down arrow asset to be more centered

Test Plan

collection.

order choice

undefined

efficient

…rse-plan into sort-by-semester

SamChou19815 · 2021-10-07T13:41:03Z

@zachary0kent You need to fix the merge conflict with the latest master to make all CI jobs run.

willespencer

This looks really good! The menu works great, and semester toggle buttons and re-ordering both seem to work great! Also tested editing semesters to ensure order is properly sorted when sorted by newest and oldest.

Left a bunch of comments, just about some code cleanup things and some bugs! I also have two misc comments...

Is the view button supposed to open the menu on hover, or on click? Not sure what the designs intended, but the hover seems a little strange to me.
Do you think compact/non-compact should persistent like semester order does to be consistent?

src/components/Semester/OrderDropdown.vue

src/assets/scss/_variables.scss

src/assets/images/schedule-view/view-settings/settings.svg

src/components/Semester/OrderDropdown.vue

src/components/Semester/OrderDropdownOption.vue

src/components/Semester/SemesterView.vue

willespencer · 2021-10-07T14:36:14Z

src/components/Semester/SemesterView.vue

+    toggleCompact(toggled: boolean) {
+      if (toggled !== this.compact) {
+        this.$emit('compact-updated', toggled);
+        GTagEvent(this.$gtag, toggled ? 'to-compact' : 'to-not-compact');


Thanks for preserving this!

hahnbeelee · 2021-10-07T22:58:56Z

There's some weird activity on mobile where the dropdown doesn't always open when you click on it. I agree with @willespencer that the dropdown should show up when you click on it instead of hovering since you can't hover on mobile.

Screen.Recording.2021-10-07.at.6.56.14.PM.mov

hahnbeelee · 2021-10-07T23:00:37Z

You can look at the RequirementsDropdown.vue code to see how to implement the dropdown appearing from clicks

src/components/Semester/OrderDropdown.vue

src/components/Semester/OrderDropdownOption.vue

github-actions · 2021-10-19T22:59:54Z

Visit the preview URL for this PR (updated for commit 9d9c91e):

https://cornelldti-courseplan-dev--pr537-sort-by-semester-hclt3wcn.web.app

_{(expires Fri, 29 Oct 2021 20:47:38 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

benjamin-shen

The merge resolutions look good! Thanks for putting so much work into this PR. It's really impressive how you picked up what was going on with Vuex.

I left some more comments on the code

src/components/Semester/Semester.vue

src/global-firestore-data/semesters.ts

benjamin-shen · 2021-10-20T00:12:50Z

src/store.ts

    setSemesters(state: VuexStoreState, semesters: readonly FirestoreSemester[]) {
-      state.semesters = semesters;
+      state.semesters = sorted(semesters, state.orderByNewest);


Won't this cause a lot of unnecessary sorting? It seems that setOrderByNewest already does this.

Yeah, I was considering the case when the the semesters collection was edited but the orderByNewest Field was unchanged; I wanted to ensure that adding/deleting a semester didn't break the sorting order. Do you think I should remove that?

src/store.ts

src/SeasonOrdinal.ts

Co-authored-by: Benjamin Shen <bfs45@cornell.edu>

hahnbeelee

oo with Toby's changes the semester title overflows in the compact view.

hahnbeelee

Zak this is really great! Can you make it so that the dropdown closes if you click anywhere on the screen? You can maybe refer to RequirementsDropdown or any of the modal code. If you find yourself spending way too much time on it though don't worry about it. Although I think clickOutside utility function might work here.

hahnbeelee · 2021-10-20T04:27:31Z

src/components/Semester/OrderDropdown.vue

@@ -0,0 +1,154 @@
+<template>


Maybe rename this file ViewDropdown

Yep I agree!

I think the wrapping issue also occurs on master

hahnbeelee · 2021-10-20T04:28:13Z

src/components/Semester/OrderDropdownOption.vue

@@ -0,0 +1,55 @@
+<template>


and this ViewDropdownOption

Thanks for the feedback! I think I fixed all of your suggestions.

hahnbeelee · 2021-10-20T17:48:05Z

@zachary0kent if you want you can address the bigger things I pointed out in a separate PR as long as it gets merged before the deploy day Nov. 4

name wrapping

willespencer

This is looking really good! All my comments were addressed, and modals now look good on top of the view text!

Still having a bit of trouble with new users though? Onboarding does not seem to be able to be finished for those users, and when clicking the "Finish" button, I get this error. Could you please look into this?

zachary-kent · 2021-10-20T23:35:52Z

This is looking really good! All my comments were addressed, and modals now look good on top of the view text!

Still having a bit of trouble with new users though? Onboarding does not seem to be able to be finished for those users, and when clicking the "Finish" button, I get this error. Could you please look into this?

Hmmm. compareFirestoreSemesters is a comparator function I removed when I reimplemented sorting. No references to it are found when I search for it in vscode, so I'm not sure where the undefined reference could be coming from. Do you have any thoughts? Also, I can replicate the issue on the staging site but not locally, which is why I didn't encounter it before; do you think that would provide some insight into the problem? I could just stub the comparator in where it was previously to solve the issue, but idk if that's ideal.

Edit: stubbing in the comparator allows you to successfully complete the tutorial on staging site.

willespencer

Works for me now so going to approve!!

But yeah I have no idea why the staging site would still expect that function to be needed.., I also don't see any references to it in the code. @hahnbeelee do you have any ideas?

benjamin-shen · 2021-10-21T01:13:45Z

@zachary0kent @willespencer

PR #529 uses compareFirestoreSemesters in a new function getActiveSemesters(). It was recently merged to master, that might be part of the issue?

I also can't reproduce locally. I would pull from master again, tweak @vaishnavi17's changes, and see if there's still an error.

also how other imports are formatted)

benjamin-shen

Thanks for addressing all my suggestions and comments!! 💯

Like we discussed, there may be a way to make this a purely frontend implementation but we can come back to this if we run into issues with this PR later (which hopefully we won't 🤞)

hahnbeelee

littt!!!!! awesome stuff zak!

zachary-kent added 18 commits October 4, 2021 19:22

Added orderByNewest field to user-semesters

fe11182

collection.

Prevented scope of breaking changes by un-nesting.

363ddd9

Updated firestore schema

1cad246

Added assets

4792347

Updated Vuex store to persist

b4aa5b5

order choice

Added check to ensure orderByNewest is not

848bde4

undefined

Basic dropdown functionality

2a932fc

Completed Dropdown Functionality

3fdf686

Made backend operations more

d929421

efficient

Added horizontal line to dropdown

b008836

Ran prettier

4f506cb

Increased horizontal line spacing

de3e835

Added type safety to event handling

562962f

Ran prettier

0200555

Removed old assets and .dmg

b48aecb

Restored old assets, removed .dmg

ca062b2

Merge branch 'sort-by-semester' of https://github.com/cornell-dti/cou…

ee0d86f

…rse-plan into sort-by-semester

Restored files

6c585d5

zachary-kent requested a review from a team as a code owner October 6, 2021 02:59

zachary-kent added 3 commits October 6, 2021 00:40

Moved sorting logic to store.ts

a43b800

Simplfied SeasonOrdinal export

d79bee9

Ran prettier

c0a484e

willespencer requested changes Oct 7, 2021

View reviewed changes

hahnbeelee reviewed Oct 7, 2021

View reviewed changes

src/components/Semester/OrderDropdown.vue Outdated Show resolved Hide resolved

hahnbeelee reviewed Oct 7, 2021

View reviewed changes

src/components/Semester/OrderDropdownOption.vue Outdated Show resolved Hide resolved

hahnbeelee reviewed Oct 7, 2021

View reviewed changes

src/components/Semester/OrderDropdownOption.vue Outdated Show resolved Hide resolved

hahnbeelee reviewed Oct 7, 2021

View reviewed changes

src/components/Semester/OrderDropdownOption.vue Outdated Show resolved Hide resolved

Removed duplicated files

8eb1a49

zachary-kent requested review from willespencer and hahnbeelee October 19, 2021 23:09

Factored out repreated code

93f4316

benjamin-shen requested changes Oct 20, 2021

View reviewed changes

zachary-kent and others added 3 commits October 19, 2021 21:10

Update src/store.ts

928d0c8

Co-authored-by: Benjamin Shen <bfs45@cornell.edu>

Refactored according to Ben's suggestions

aa620b9

Fixed persistence

2aaa33a

hahnbeelee suggested changes Oct 20, 2021

View reviewed changes

zachary-kent added 3 commits October 20, 2021 15:57

Renamed OrderDropdown to ViewDropdown

b757494

Added no-wrap rule to prevent semester

e5398dd

name wrapping

Made dropdown close upon outside click

0856739

zachary-kent requested a review from hahnbeelee October 20, 2021 20:19

Reverted Semester.vue to master

054ce30

willespencer requested changes Oct 20, 2021

View reviewed changes

Added compareFirestoreSemesters back

23cc2bb

willespencer approved these changes Oct 21, 2021

View reviewed changes

zachary-kent added 3 commits October 22, 2021 16:24

Merge branch 'master' into sort-by-semester

2b1da10

Changed setOrderByNewest to be pure

d72f7a8

Removed @ prefix (seemed to break frontend test,

9d9c91e

also how other imports are formatted)

zachary-kent requested a review from benjamin-shen October 22, 2021 20:48

benjamin-shen approved these changes Oct 23, 2021

View reviewed changes

hahnbeelee approved these changes Oct 23, 2021

View reviewed changes

zachary-kent merged commit 10764a2 into master Oct 23, 2021

zachary-kent deleted the sort-by-semester branch October 23, 2021 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort by semester #537

Sort by semester #537

zachary-kent commented Oct 6, 2021

SamChou19815 commented Oct 7, 2021

willespencer left a comment

willespencer Oct 7, 2021

hahnbeelee commented Oct 7, 2021 •

edited

Loading

hahnbeelee commented Oct 7, 2021

github-actions bot commented Oct 19, 2021 •

edited

Loading

benjamin-shen left a comment

benjamin-shen Oct 20, 2021

zachary-kent Oct 20, 2021

hahnbeelee left a comment

hahnbeelee left a comment

hahnbeelee Oct 20, 2021

zachary-kent Oct 20, 2021

zachary-kent Oct 20, 2021

hahnbeelee Oct 20, 2021

zachary-kent Oct 20, 2021

hahnbeelee commented Oct 20, 2021

willespencer left a comment •

edited

Loading

zachary-kent commented Oct 20, 2021 •

edited

Loading

willespencer left a comment

benjamin-shen commented Oct 21, 2021 •

edited

Loading

benjamin-shen left a comment

hahnbeelee left a comment

Sort by semester #537

Sort by semester #537

Conversation

zachary-kent commented Oct 6, 2021

Summary

Test Plan

SamChou19815 commented Oct 7, 2021

willespencer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hahnbeelee commented Oct 7, 2021 • edited Loading

hahnbeelee commented Oct 7, 2021

github-actions bot commented Oct 19, 2021 • edited Loading

benjamin-shen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hahnbeelee left a comment

Choose a reason for hiding this comment

hahnbeelee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hahnbeelee commented Oct 20, 2021

willespencer left a comment • edited Loading

Choose a reason for hiding this comment

zachary-kent commented Oct 20, 2021 • edited Loading

willespencer left a comment

Choose a reason for hiding this comment

benjamin-shen commented Oct 21, 2021 • edited Loading

benjamin-shen left a comment

Choose a reason for hiding this comment

hahnbeelee left a comment

Choose a reason for hiding this comment

hahnbeelee commented Oct 7, 2021 •

edited

Loading

github-actions bot commented Oct 19, 2021 •

edited

Loading

willespencer left a comment •

edited

Loading

zachary-kent commented Oct 20, 2021 •

edited

Loading

benjamin-shen commented Oct 21, 2021 •

edited

Loading