-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Fix #20003: Adjust "My Dashboard" button's position when "Review Lowest Scored Skill" button appears #20156
base: develop
Are you sure you want to change the base?
Conversation
Hi @LKMartin12, can you complete the following:
|
Assigning @Nik-09 for the first pass review of this PR. Thanks! |
Hey @Nik-09, PTAL at this PR, Thanks! |
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.
Hi @LKMartin12 Can you please add before and after changes, this helps the reviewers to understand the UI changes quickly.
Thanks
Hi @LKMartin12, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
Hi @Nik-09 sorry for the late response! As requested here is how the code looked like before |
Hi @LKMartin12, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
@Nik-09 When you are available could you take a look at it? The bot is trying to close it ahaha, Thank You! |
@LKMartin12 Please see the last point of the "essential checklist" in the description of the PR. There is a specific syntax you need to use when assigning PRs to others -- if you don't do that, then they won't see it. Try that and feel free to let us know if it doesn't work. Thanks! |
Hi @LKMartin12, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
Ok @seanlip , I´ll take a look and update it as soon as I can |
Hi @LKMartin12, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
@{{Nik-09}}PTAL |
@Nik-09 PTAL |
Unassigning @LKMartin12 since a re-review was requested. @LKMartin12, please make sure you have addressed all review comments. Thanks! |
Sorry, @LKMartin12 for the delay. I will take a look tomorrow morning. |
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.
Hi @LKMartin12 thank you for creating the PR. Can you also add a screenshot for these two cases:
(a). a logged-out learner answers all the questions correctly
(b). a logged-out answers a few questions correctly.
Thank you
Hi @LKMartin12, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
Sorry for the delayed response! Will do that as soon as I can |
b16c5fe
to
57f49d9
Compare
Overview
This PR fixes The ‘My Dashboard’ button is not positioned correctly when the ‘Review Lowest Scored Skill’ button appears in the Oppia Maths Classroom/Practice Session on the test server.” #20003 .
This PR does the following: The previous implementation of the file question-player.component.html was making two footers, in which there would be two buttons in the first footer and only one in the second footer.
But the list used to iterate through how many buttons would be placed in each footer was iterating a list of three element (contained all three buttons in it) which was making the css property justify-content leave space for a third button in the first footer (it isn't a problem to do this in the second footer as we want to have only one button on the right side, which will happen as long another button is considered prior to him).
This led to the "Review Lowest Scored Skill" button being correctly placed but, as space was left for a third button, the "My Dashboard" button wouldn't stick to the right. In order for this to happen, I made it so that we use the same list (so that we don't have to add new methods to the ts file or tests for it) and only iterate through the first two elements, which will always be the buttons in the first footer.
If the button "Review Lowest Scored Skill" doesn't appear it is still important to continue counting him, as we want to make the "My Dashboard" button stick to the right either way.
Proof that changes are correct
Essential Checklist