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

[#12658] Instructor Home Page: Dropdown buttons on mobile #12662

Merged
merged 13 commits into from
Dec 24, 2023

Conversation

Tejasker
Copy link
Contributor

Fixes #12658

Outline of Solution
Made Responsive Drop-down Menu for the Instructor Home Page

Before there was an Overflow of the drop-down buttons on the home page

Screenshot 2023-12-16 113322-instructor
Screenshot 2023-12-16 113409-sessions

After making it Responsive according to the size of the screen, I am able to fix the overflow(spilling out) issue

Modified Screens
Screenshot 2023-12-16 113640
Screenshot 2023-12-16 113720

  • Tested with all screen widths.
  • Run all the test cases including lint cases & passed

@weiquu
Copy link
Contributor

weiquu commented Dec 16, 2023

Hi @Tejasker, can you check out the failing tests? You might need to check out the relevant spec.ts file for the failing component tests (should be instructor-home-page.component.spec.ts), and the InstructorHomePageE2ETest.java file for the failing E2E tests. You can see more details on where it failed by clicking on the "Details" button next to the failed checks.

I've taken a quick look and it seems that the tests can't find the archive and delete buttons by the .btn-archive-course and .btn-delete-course classes. I assume it's something to do with the container they are in (i.e. they are no longer a child of the component we are looking at), but I'm not too sure. Do check on this and let us know if you are facing any issues (and what you have tried).

There are also a couple of snapshot tests that are failing. You can update the snapshots by running npm run test and pressing a to run all test cases. After that, check through the snapshots to make sure the changes are as expected, then press u to update them. You can find more details on snapshot testing here.

@weiquu weiquu added the s.Ongoing The PR is being worked on by the author(s) label Dec 16, 2023
@Tejasker
Copy link
Contributor Author

hii @weiquu, I have made updates in the snapshot by running the npm test & checked with the component test failing cases. These component tests are now passed after updating the snapshot & also E2E tests .

@weiquu
Copy link
Contributor

weiquu commented Dec 17, 2023

Hi @Tejasker, thanks for fixing the snapshot tests. Note that the other tests are still failing - please refer to the first 2 paragraphs of my previous comment for some guiding info.

In particular, the container="body" line is causing the dropdown to be added as the last child of the body element. This means that previous ways to find the button for testing no longer work. You can take a look at the Instructor Feedback Sessions page for an example. Lines 243-258 of InstructorFeedbackSessionsPage.java should also give you an idea of how to fix the E2E tests - these should then be transferrable to the component tests.

@Tejasker
Copy link
Contributor Author

hii @weiquu , i have manged to solve the Component test that are causing test cases to fail in CI , I will be further working on the E2E test that Is making CI fail , if possible I required your help in solving this issue if you can guide to over come the E2E test it would be much appreciated. Anyhow i will try my best to solve the E2E test failure.

Thanks

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.

Hi @Tejasker, great work on the tests! Almost good to go, just a few small nits.

@Tejasker
Copy link
Contributor Author

Hii @weiquu once you are OK with my above responses for the change requested, Then I can raise a PR

@weiquu
Copy link
Contributor

weiquu commented Dec 20, 2023

Hii @weiquu once you are OK with my above responses for the change requested, Then I can raise a PR

Hi @Tejasker, replying to the E2E and component tests comments here:

I think my suggestion of body > div > div > .btn-delete-course (and similarly for .btn-archive-course) works because we are still directing the query down the same path to the button. I've checked that this works with a test PR which you can view here. You can verify that all checks pass, and under the files changed section, that I've used body > div > div > .btn-delete-course. If I'm not wrong, querySelector needs a reference to the parent element of the element we are finding, which is why the examples you've given above do not work.

Let me know if the above makes sense. If it's okay, please go ahead and make the changes to the same branch (i.e. use this same PR)

@Tejasker
Copy link
Contributor Author

Hii @weiquu as mentioned by you above regarding the changes, I have made the necessary changes ( ie small adjustments & alignments in the code) and merged the 2 commits ahead of the my main repo #12671, #12667, and made my forked repo in- sync with the original . I Have raised PR with the same branch as earlier.

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.Ongoing The PR is being worked on by the author(s) labels Dec 20, 2023
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. Great work, and thank you for your contribution!

@domlimm domlimm added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Dec 24, 2023
@domlimm domlimm merged commit 1915ade into TEAMMATES:master Dec 24, 2023
9 checks passed
@wkurniawan07 wkurniawan07 added the c.Feature User-facing feature; can be new feature or enhancement to existing feature label 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
…EAMMATES#12662)

* fixed changes & made responsive drop down for the indtructor home page

* resolved the issues that are causing test cases that are failing

* resolved the Component testing issues that is causing test cases fail.

* resolved the E2E  testing issues.

* resolved the E2E  testing issues.

* resolved the E2E  testing issues.

* resolved the E2E  testing issues.

* resolved the E2E  testing issues.

* resolved the E2E  testing issues.

* modified & made small adjustments in the code as required

---------

Co-authored-by: Jason Qiu <[email protected]>
Co-authored-by: Dominic Lim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Instructor Home Page: Dropdown buttons on mobile
5 participants