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

[#12269] Instructor edit sessions page: Fix add question button overflow. #12477

Merged
merged 15 commits into from Jul 2, 2023

Conversation

singhabhyudita
Copy link
Contributor

Fixes #12269

Outline of Solution

Used breakpoints and inbuilt padding classes of Bootstrap to reduce the padding at smaller screen width.

Here are the results:

  • Instructor edit sessions page
Screenshot 2023-06-16 at 2 19 06 PM
  • Instructor help page
Screenshot 2023-06-16 at 2 18 23 PM

@github-actions
Copy link

Hi @singhabhyudita, thank you for your interest in contributing to TEAMMATES!
However, your PR does not appear to follow our contribution guidelines:

  • Title must start with the issue number the PR is fixing in square brackets, e.g. [#<issue-number>]

Please address the above before we proceed to review your PR.

@singhabhyudita singhabhyudita changed the title [12269] Instructor edit sessions page: Fix add question button overflow. [#12269] Instructor edit sessions page: Fix add question button overflow. Jun 16, 2023
@weiquu
Copy link
Contributor

weiquu commented Jun 17, 2023

Hi @singhabhyudita, do take a look at the failing tests before we proceed to review your changes.

In addition, I believe the results in the screenshot don't address the issue raised, which is the overflowing of buttons into the next line. That is to say, the solution should be to have the buttons be on the same line regardless of screen width (either by reducing the padding between components at smaller screen widths or other means). (@zhaojj2209 do lmk if I misunderstood)

@singhabhyudita
Copy link
Contributor Author

Hi @weiquu , could you please tell me how do I fix these failing checks?

Also after going through the issue the first time I thought by overflowing you meant that the buttons are overflowing into one another so I thought the padding of the buttons needs to be reduced at smaller screen width.

And before this I tried to use custom styling to increase the margins and @domlimm suggested that it was fine but I could use bootstrap classes to do the same, so I thought I understood the problem correctly.

Anyways now there is no confusion so I will try to fix it as soon as possible. Kindly help me with the checks because I am confused so as to how I fix the failing checks.

Thank you in advanced!

@singhabhyudita
Copy link
Contributor Author

Hi @weiquu , is this what you are referring to?

This screenshot is of screen width 320px.
Screenshot 2023-06-18 at 7 58 41 PM

@weiquu
Copy link
Contributor

weiquu commented Jun 19, 2023

Hi @singhabhyudita, you can read more about snapshot testing here. 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.

Please note that as per our mobile-friendliness best practices, the minimum screen width we support is 360px.

The idea for the solution is fine; do let me know when your PR is ready for review.

@singhabhyudita
Copy link
Contributor Author

@weiquu kindly review the changes!

@domlimm domlimm added the s.ToReview The PR is waiting for review(s) label Jun 20, 2023
@weiquu
Copy link
Contributor

weiquu commented Jun 20, 2023

Not sure if having the buttons be of different heights is desirable... a quick comparison of different solutions (current solution; reducing padding at the end; reducing padding of the info icon and at the ends):
Screenshot 2023-06-21 at 1 37 24 AM
Screenshot 2023-06-21 at 1 38 00 AM
Screenshot 2023-06-21 at 1 39 01 AM

Any thoughts? Also pinging @zhaojj2209 @domlimm and any other contributors :')

In any case, I think the changes shouldn't be made in .background-color-light-blue. It has a specific use and we should try to avoid any cases of unintended side effects

@singhabhyudita
Copy link
Contributor Author

I thought the current solution was acceptable since I had confirmed it with @weiquu beforehand.
Nonetheless @domlimm @zhaojj2209 could you please help me out with the desired solution?

@zhaojj2209
Copy link
Contributor

I would suggest the third solution (reduce padding both at the end and for the info button).

@weiquu
Copy link
Contributor

weiquu commented Jun 21, 2023

Thanks @zhaojj2209!

@singhabhyudita, for your reference, I reduced the padding for both by setting padding-left and padding-right to 5px (if I remember correctly). You can play around with the number to see what works best (or think of a solution that doesn't need to set a fixed padding)

@singhabhyudita
Copy link
Contributor Author

Have updated the changes. Kindly review.
(@zhaojj2209 @domlimm @weiquu)

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.

Couple of small comments, should be good to go after changes

@weiquu weiquu requested a review from domlimm June 26, 2023 19:25
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

@singhabhyudita
Copy link
Contributor Author

Pinging @domlimm for review!

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 contributing! So sorry for the delay

@domlimm domlimm merged commit 14dde85 into TEAMMATES:master Jul 2, 2023
8 checks passed
Zxun2 pushed a commit to Zxun2/teammates that referenced this pull request Jul 12, 2023
…ton overflow. (TEAMMATES#12477)

* fixed overlapping button issue

* Overflow fixed

* Updating failing checks

* Reduced padding at ends and for the info button

* Corresponding snapshots updated

* Padding changed to horizontal axis

* snapshots updated

* custom styling added
@samuelfangjw samuelfangjw added c.Bug Bug/defect report s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.ToReview The PR is waiting for review(s) labels 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 edit sessions page: Fix add question button overflow
6 participants