Fix issue #13699: fix response tooltip misplaced and image doesn't show up #20147
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.
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! |
|
Hi @seanlip, I've already added tests related to the code changes that allow the graphs resize. |
|
Unassigning @ana-mc-almeida since a re-review was requested. @ana-mc-almeida, please make sure you have addressed all review comments. Thanks! |
|
@ana-mc-almeida Sorry for the late reply. Yup, this is a known flake: #19732. I'll restart the failing test. In general, if a CI check fails in future, please check the existing issues to see whether it's been reported. If so, link to that issue and ask for a re-run. If not, then the error might be due to your PR or a new issue might need to be filed. Thanks! |
| @@ -57,6 +57,7 @@ export class InteractionDisplayComponent { | |||
|
|
|||
| ngAfterViewInit(): void { | |||
| this.buildInteraction(); | |||
| this.changeDetectorRef.detectChanges(); | |||
There was a problem hiding this comment.
Can you explain (for posterity) why this change was needed?
There was a problem hiding this comment.
I had already justified this here, but now that I've looked back at the code, I think it's not necessary since the changes are already being detected within the buildInteraction(). I'll remove it.
|
|
||
| var maxY = this.getMaxY(); | ||
| if (this.HEIGHT < maxY) { | ||
| scale = this.HEIGHT / (maxY + this.MIN_LEFT_MARGIN); |
There was a problem hiding this comment.
Why are we comparing height to MIN_LEFT_MARGIN ... shouldn't it be MIN_TOP_MARGIN or similar?
There was a problem hiding this comment.
Sorry, you're absolutely right. This name stems from previous changes where only left margin was used. I'll change the name to MIN_MARGIN since it's being used everywhere there's resizing to maintain consistency.
There was a problem hiding this comment.
Hm, just to check, conceptually why is the min left margin equal to the min top margin?
There was a problem hiding this comment.
I only saw your response after committing the changes, I apologize. I'll change it if you think it's necessary. However, I decided to make the margins equal purely for aesthetic reasons, as I believe it looks better than having the graph just with left margin.
There was a problem hiding this comment.
OK, do we use these as right and bottom margins too?
If not, you can call it MIN_LEFT_AND_TOP_MARGIN; we can split it out later if needed.
There was a problem hiding this comment.
Wait, actually .. I suppose it's kind of weird to only have a margin on the top and the left. How do you ensure that the graph doesn't extend too far to the right or bottom?
There was a problem hiding this comment.
Because when that happens, I'm resizing the graph.
There was a problem hiding this comment.
Sorry @ana-mc-almeida, I still don't get it.
What is the exact definition of MIN_MARGIN? What does it represent, exactly?
Conceptually, I would have expected to see the bounding box computed based on something like (maxX - minX) + padding and similar for Y. Not saying what you've done is wrong, just that I don't understand it -- I'm trying to better understand why we are treating e.g. the left vs right margins differently. Can you elaborate a bit more on the math behind your approach?
There was a problem hiding this comment.
@seanlip sorry for the delay in responding, I will walk through my thought process to see if I can answer your questions:
The first thing I do to avoid the scrollbar appearing is to remove the white space to the left of the graph. Here arises the first necessity of using the MIN_MARGIN: I want to leave a small space so that the nodes are not completely at 0, as it appears here:
In this way, I am setting the default value of MIN_MARGIN = 10. However, for Arabic languages, the label appears before the node, so it is necessary for the MIN_MARGIN to vary according to the label in the case of a labeled graph.
After removing the white space, if it is still necessary to reduce the image, it is necessary to obtain the scale. Here, to avoid the graph becoming disproportionate, I will calculate the scale for both height and width. Again, because of the labels (this time for English, for example), it is necessary to consider the already calculated MIN_MARGIN. To ensure consistent scaling (without the graph becoming disproportionate), I use the margin for both the height and width scaling.
I don't think this variable can be exactly considered as padding because, if it were, either the size of the popup would vary, or the graph would become disproportionate.
I think I have explained the entire thought process behind my code, especially how I am resizing the graph so that it does not extend too much to the right or downwards, but if you still have doubts, I will try other ways to explain my code.
I apologize for the delay in responding; I was unable to use my computer for a few days and ended up being quite overwhelmed with university and work. I also had to take extra time to do some manual tests to see how the graphs would look if I were calculating this margin in a different way and I really believe this is the best solution.
While doing these tests, I realized that I can also remove the extra white space at the top, and the graphs now look like this:
Initially, this was not done because there was no need to remove the vertical scroll, but I think this way the graph looks even better. I hope this doesn't confuse you even more regarding the initial doubts about why the MIN_MARGIN is necessary, but let me know if you want me to implement this.
Once again, sorry for this confusion and, even worse, for the delay in responding, and thank you very much for your attention with this issue.
|
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. |
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