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

[#12344] Instructor courses page: fix margin of copy instructors from other courses modal on mobile #12389

Merged

Conversation

b-walton
Copy link
Contributor

Fixes #12344

Outline of Solution

Fixes mobile margin by only applying width constraint on media 768 pixels or wider.
(ready for review)

Desktop View:
image

Mobile View:
image

@weiquu
Copy link
Contributor

weiquu commented Apr 18, 2023

Hi @b-walton, solution looks fine at a glance, but do fix the lint issues before we do a complete review.

@zhaojj2209 zhaojj2209 added the s.ToReview The PR is waiting for review(s) label Apr 19, 2023
@zhaojj2209 zhaojj2209 requested a review from weiquu April 19, 2023 06:40
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 @b-walton, while this solution works, I think we should implement it in a way that is consistent with the rest of the modals. In particular, you can take a look at the copy-questions-from-other-sessions-modal component to see what I mean (involves setting min-width instead). Do let me know if you require any clarification.

Don't forget to update your branch with the latest changes, and re-request a review when you're ready!

@b-walton
Copy link
Contributor Author

Are there any likely reasons for why the E2E tests fail?

@weiquu
Copy link
Contributor

weiquu commented Apr 22, 2023

Are there any likely reasons for why the E2E tests fail?

Hmm E2E tests can fail (for frontend changes) if there are changes to page structure / behaviour / element identifiers. However, they might occasionally fail due to the tests being unstable, in which case we will run them again (: Your PR passes all checks, so no worries there.

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! Thanks for contributing

@weiquu weiquu added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Apr 22, 2023
Copy link
Contributor

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

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

LGTM

@zhaojj2209 zhaojj2209 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 Apr 23, 2023
@zhaojj2209 zhaojj2209 merged commit 69c2a90 into TEAMMATES:master Apr 23, 2023
7 checks passed
@samuelfangjw samuelfangjw added the c.Bug Bug/defect report label Jul 14, 2023
@samuelfangjw samuelfangjw added this to the V8.28.0 milestone Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Bug Bug/defect report 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 courses page: fix margin of copy instructors from other courses modal on mobile
4 participants