-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
[#12287] Student home page: Make it easier to navigate courses #12493
[#12287] Student home page: Make it easier to navigate courses #12493
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM but got a query about the testing part. Let's also wait for @wkurniawan07 on allowing students to see course creation and deletion time.
Could you also update your branch with the latest master?
src/web/app/pages-student/student-home-page/student-home-page.component.spec.ts
Outdated
Show resolved
Hide resolved
Hmm 🤔 Should i be worried about the failing E2E Tests? It is passing on my fork. |
@Zxun2 It fails sometimes, and we will re-run it. If not, we will need you to look into it. No worries ZX! |
@domlimm Alright, thanks dom! 😊 |
Bumping @wkurniawan07! |
Any update on this? |
Hi @Zxun2, we're still waiting on @wkurniawan07 to weigh in on the hiding/unhiding of the creation time information. Also pinging other senior members @zhaojj2209 and @samuelfangjw for help :') |
From looking at the issue #10264 thread, both creation and deletion timestamp are labelled as "good to hide" instead of "must hide", so at the very least exposing them won't cause serious security issues. My take is that it should be safe to expose creation timestamp for the sake of sorting courses by creation date. It would be good if we can continue hiding deletion timestamp though, let's not expose more than necessary. |
Idea behind hiding these attributes is to, as a general principle, expose only as little as necessary for the app to function since we can never be sure how different bits of information can be used in an attack. I'm ok with exposing creation time since it is required for this functionality and I doubt exposing this attribute is inherently insecure. |
@samuelfangjw @zhaojj2209 @weiquu @NicolasCwy Thanks for the inputs! I will make the changes shortly :) |
…xun/add-ability-to-sort-by-date
…com/Zxun2/teammates into zongxun/add-ability-to-sort-by-date
…com/Zxun2/teammates into zongxun/add-ability-to-sort-by-date
Hi @Zxun2, looks great (: Can I just check 1 small thing:
Wondering if I've understood this correctly (seems to be the behaviour I saw as well), and if so, whether this difference was intentional Anyway, didn't spot any other issues so all's good! |
…com/Zxun2/teammates into zongxun/add-ability-to-sort-by-date
@weiquu Thanks for the catch! That was not intentional 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for working on this @Zxun2 and sorry for the initial review delays!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes #12287
Outline of Solution
For students who link multiple courses to the same account, the number of courses on the home page may build up over the years, making it difficult to find a specific course.
Currently, these courses and their feedback sessions are loaded immediately on page load, resulting in long loading periods.
This PR introduces lazy loading to courses, loading feedback sessions only when needed. In addition, an accordion is used to make the UI cleaner (the top 3 (or 5?) most recent courses are open by default).
Overview of changes
Demo
Sort.Courses.by.Creation.Date.and.Lazy.Loading.mp4