-
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
[#12269] Instructor edit sessions page: Fix add question button overflow. #12477
Conversation
Hi @singhabhyudita, thank you for your interest in contributing to TEAMMATES!
Please address the above before we proceed to review your PR. |
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) |
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! |
Hi @weiquu , is this what you are referring to? |
Hi @singhabhyudita, you can read more about snapshot testing here. You can update the snapshots by running 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. |
@weiquu kindly review the changes! |
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): Any thoughts? Also pinging @zhaojj2209 @domlimm and any other contributors :') In any case, I think the changes shouldn't be made in |
I thought the current solution was acceptable since I had confirmed it with @weiquu beforehand. |
I would suggest the third solution (reduce padding both at the end and for the info button). |
Thanks @zhaojj2209! @singhabhyudita, for your reference, I reduced the padding for both by setting |
Have updated the changes. Kindly review. |
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.
Couple of small comments, should be good to go after changes
src/web/app/components/adding-question-panel/adding-question-panel.component.html
Outdated
Show resolved
Hide resolved
src/web/app/components/adding-question-panel/adding-question-panel.component.html
Outdated
Show resolved
Hide resolved
src/web/app/components/adding-question-panel/adding-question-panel.component.html
Outdated
Show resolved
Hide resolved
Pulling from upstream
Updating the changes in the forked repository
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 contributing
Pinging @domlimm for review! |
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! Thank you for contributing! So sorry for the delay
…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
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: