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

[#12656] Student Home Page: Indicate default sort #12660

Merged
merged 8 commits into from
Dec 24, 2023
Merged

[#12656] Student Home Page: Indicate default sort #12660

merged 8 commits into from
Dec 24, 2023

Conversation

averheecke-scottlogic
Copy link
Contributor

Fixes #12656

Outline of Solution
The problem was that the default sortBy was set to NONE. Changed this to COURSE_CREATION_DATE to result in the desired functionality.

@domlimm
Copy link
Contributor

domlimm commented Dec 14, 2023

Hello @averheecke-scottlogic, please fix the failing snapshot test before we proceed to review your PR. You may update it by running npm run test in the root directory of the project. You may find more details on snapshot testing here.

@jasonqiu212 jasonqiu212 added the s.ToReview The PR is waiting for review(s) label Dec 16, 2023
@weiquu
Copy link
Contributor

weiquu commented Dec 16, 2023

Hi @averheecke-scottlogic, thanks for the PR! The button now shows that the default sort is by creation date, however I don't think that that is the actual behaviour.

Here is my student home page immediately after loading (I've closed the panels for clarity):
Screenshot 2023-12-17 at 1 31 34 AM

Upon clicking on the "Course ID" button, the sort remains exactly the same. When I click back onto the "Creation Date" button, the sort becomes as such:
Screenshot 2023-12-17 at 1 31 48 AM

It appears that even though the button indicates that the default sort is by creation date, the sort is still by Course ID. Could you please take a look?

@averheecke-scottlogic
Copy link
Contributor Author

Hi @weiquu, the correct desired functionality should now be added :)

weiquu
weiquu requested changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@weiquu weiquu 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! Just 1 small change

src/web/app/pages-student/student-home-page/student-home-page.component.ts Outdated Show resolved Hide resolved
weiquu
weiquu approved these changes Dec 20, 2023
View reviewed changes
Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

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

LGTM

@weiquu weiquu added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Dec 20, 2023
domlimm
domlimm approved these changes Dec 24, 2023
View reviewed changes
Copy link
Contributor

@domlimm domlimm left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for your contribution!

@domlimm domlimm merged commit b44938b into TEAMMATES:master Dec 24, 2023
9 checks passed
@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging c.Feature User-facing feature; can be new feature or enhancement to existing feature and removed s.FinalReview The PR is ready for final review labels Jan 21, 2024
@wkurniawan07 wkurniawan07 added this to the V8.30.0 milestone Jan 21, 2024
cedricongjh pushed a commit to cedricongjh/teammates that referenced this pull request Feb 20, 2024
@averheecke-scottlogic @domlimm @cedricongjh
[TEAMMATES#12656] Student Home Page: Indicate default sort (TEAMMATES…
686eb73 
…#12660)

* sortBy to CREATION DATE

* snapshot update

* sort to creation time upon load

* linting to specs

* trailing space fix

* replaces sort with sort function

---------

Co-authored-by: Dominic Lim <46486515+domlimm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domlimm domlimm domlimm approved these changes

@weiquu weiquu weiquu approved these changes

@cedricongjh cedricongjh Awaiting requested review from cedricongjh

Assignees
No one assigned
Labels
c.Feature User-facing feature; can be new feature or enhancement to existing feature s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V8.30.0
Development

Successfully merging this pull request may close these issues.

Student Home Page: Indicate default sort
5 participants
@averheecke-scottlogic @domlimm @weiquu @wkurniawan07 @jasonqiu212