-
-
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 issue #13699: fix response tooltip misplaced and image doesn't show up #20147
base: develop
Are you sure you want to change the base?
Conversation
Assigning @Lawful2002 for the first pass review of this PR. Thanks! |
Hi! @ana-mc-almeida Welcome to Oppia! Could you please follow the instructions here and sign the CLA Sheet to get started? You'll need to do this before we can accept your PR. Thanks! |
@seanlip PTAL |
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.
Thanks @ana-mc-almeida! Left a couple of notes. Also could you please look at the failing CI checks?
...lates/pages/exploration-player-page/learner-experience/input-response-pair.component.spec.ts
Outdated
Show resolved
Hide resolved
extensions/interactions/GraphInput/directives/oppia-response-graph-input.component.spec.ts
Outdated
Show resolved
Hide resolved
Also @ana-mc-almeida one other question. Could you avoid the reader having to scroll the graph in the response? (I looked at your video.) Also how does the behaviour on mobile look like? Thanks! |
Hi @seanlip, thanks for your reply!
I had also thought about this and even commented a proposed solution in the issue, but after testing the interactions, I noticed that it couldn't be done because it would affect the lessons already created since it depends on the position where the graph starts to be drawn. Let me know if you think there's any other way to do this.
Thank you in advance for your time. |
@ana-mc-almeida Some thoughts -- perhaps you could determine the bounding box for the graph by looking at the coordinates of the vertices in it, and that will tell you what to show. Another option could be to show the full size of the graph canvas but "zoomed out" so that it's smaller (but still rectangular). I think both of those approaches would generalize for all the graphs, would any of the approaches work? |
Hi @seanlip, sorry for the late reply and thanks for your help. I think I have found a solution to remove the unnecessary whitespace that was causing users to have to scroll on the graph, but I would like to have your opinion on the way I'm doing it before committing the changes. I followed your suggestion to check the coordinates of the graph vertices, and thus I am finding the vertex with the smallest X and only drawing from there. In the case of a labeled graph, it is also necessary to consider the labels. Below are screenshots of the changes: This way, we end up with these graphs: Please let me know if this is the result you were expecting. |
@ana-mc-almeida Generally speaking, the results in the pop-ups look fine, except that the whole point of this was to remove the scrollbars in the pop-ups, either by cropping or by shrinking the image or by making the pop-up a bit bigger. Could that be fixed? Also, for drag-and-drop, you might want to remove the whitespace on the left in the pop-up. Otherwise it would not look great if the choices were longer. Thanks! |
@seanlip, sorry for misunderstanding. However, I'm not understanding the goal of removing the scrollbars: increasing the size of the popup may lead to other problems, especially on mobile, and reducing the graph could make it illegible. In your opinion, how should the popup look for the following graph? Screencast_20240420_154402.online-video-cutter.com.mp4I apologize deeply for this confusion. |
@ana-mc-almeida In my opinion, the graph should be zoomed out so that a smaller version shows in the popup without scrollbars. Or the popup should be bigger so that it doesn't have scrollbars. |
Hi @ana-mc-almeida, 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. |
@seanlip apologies for the delayed response, I was facing some difficulties, but I believe I've managed to sort them out. |
@ana-mc-almeida Your screenshots look good to me. Thanks for checking! |
Unassigning @ana-mc-almeida since a re-review was requested. @ana-mc-almeida, please make sure you have addressed all review comments. 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 @ana-mc-almeida, could you please respond to all review comments, as explained in the PR guidelines? Please don't resolve comments directly, it makes it hard for reviewers because they have to open each comment to get the context.
Please fix this and then request review again. Thanks!
Sorry @seanlip, I've already unresolved and responded to all comments. Could you PTAL? Thanks! |
Unassigning @ana-mc-almeida since a re-review was requested. @ana-mc-almeida, please make sure you have addressed all review comments. 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.
LGTM. Thanks @ana-mc-almeida !
@Lawful2002 @lkbhitesh07 PTAL for codeowners, thanks. |
Hi @ana-mc-almeida. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks! |
@Lawful2002 @lkbhitesh07 friendly reminder please, this PR is getting stale -- thanks. |
Unassigning @Lawful2002 since they have already approved the PR. |
Head branch was pushed to by a user without write access
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.
Thanks @ana-mc-almeida for the changes, LGTM for the code owner files.
Hi @ana-mc-almeida, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks! |
Hi @seanlip , I am failing some frontend tests, but I don't think they are flaky nor caused by changes in my PR. Could you please help me? Thanks in advance. |
Overview
Essential Checklist
Please follow the instructions for making a code change.
Proof that changes are correct
Before:
After:
Screencast_20240410_132208.webm
As requested by @seanlip in the discussion of issue #13699, the video also includes proof that other types of interactions (such as drag-and-drop) are functioning properly.
Proof of changes in Arabic language
PR Pointers