Update popover() method to prevent its untimely dismissal #227

Git-Harshit · 2021-08-01T16:07:46Z

Checklist

I indicated which issue (if any) is closed with this PR using closing keywords
I only changed lines related to my PR (no bulk reformatting)
I indicated the source and check the license if I re-use code, or I did not re-use code
I made my best to solve only one issue in this PR, or explain why multi had to be solved at once.

Issue

fix #225.
Do check the working GIF for the proposed change on this Issue (#225).

Details

Updated popover() methods in tree-edam-stand-alone.js file by passing container option to it in order to prevent its dismissal when the mouse is moved to the popover content, from the anchor tag that launched it.

Providing ':scope' as selector string in container property allows popover element to be placed under the ':scope' or the current (here, <a>) element that called it, thus making the popover <div> a child element of <a>, thus letting hover on popover to maintain mouseover hover on the triggering <a> tag, and preventing its untimely dismissal. Additionally, passing trigger: "hover focus" option allows the popover to trigger on either mouse hover, or on focus on the element.

Please check it out, and let me know if any updations or additional information is required for it.

bryan-brancotte

Hi, thanks @Git-Harshit for this PR, I have some remarks that you will find attached, and also I have a graphical bug : when hovering biotools line, it is biosphere who is triggered.

Additionally, and maybe it's a compatibility issue, but I don't see in Chrome 91 that the patch is fixing the issue, as you can see in the attached gif.

bryan-brancotte · 2021-08-02T10:29:21Z

js/tree-edam-stand-alone.js

@@ -441,7 +441,7 @@ function interactive_edam_browser(){
                 to_biosphere_href(c[0],caller_s.get_url(),data[0]) + ' by appliances, ' +
                 to_biosphere_href(c[1],caller_s.get_url(),data[1]) + ' by tools.' +
                 '</span>').appendTo(elt);
-                $('#details-'+identifier+' .'+id_s+' [data-toggle="popover"]').popover();
+                $('#details-'+identifier+' .'+id_s+' [data-toggle="popover"]').popover({trigger: 'hover focus', container: ":scope"});


Triggering also on focus is a good idea, but should be done when the popover is written (in once place) instead on when it is instanciated (4 times). The part was not readable at the time, I improved it and you can see [here](https://github.com/edamontology/edam-browser/blob/main/js/tree-edam-stand-alone.js#L256-L261 that you can change properties for all link with popover

bryan-brancotte · 2021-08-02T11:25:07Z

Hi again,

I am not aware of :scope I most of the time use mouseover/mouseout as you can see here

Git-Harshit · 2021-08-02T12:50:43Z

Hi @bryan-brancotte, thank you for your detailed review. After looking into it further, I too have found out that :scope isn't really functioning as expected, so as a quick fix, I have updated the container option from :scope to your same code that specifies the selector before .popover() method. Now, it seems to function better on individual hover, as demonstrated in the GIF Below:

However, there still seems to be a small issue around the biosphere popovers. Moving mouse from first biosphere link to its popover seems to close its own popover and launch popover of the second link to its right (This is visible at the end of above GIF, if you could observe). Please see if this issue is reproducible at your end as well. Rest seems to be working fine.

Yes, the mouseover/mouseout example in your follow-up seems to have the right behavior of the popover. While I try to understand the current code snippet and its conversion to your given example, feel free to update the code, and let me know if any other changes are needed from my end.

bryan-brancotte

We are getting close ! Thanks for finding the container, and how to make it works !

bryan-brancotte · 2021-08-03T09:06:01Z

js/tree-edam-stand-alone.js

@@ -446,7 +446,7 @@ function interactive_edam_browser(){
                 to_biosphere_href(c[0],caller_s.get_url(),data[0]) + ' by appliances, ' +
                 to_biosphere_href(c[1],caller_s.get_url(),data[1]) + ' by tools.' +
                 '</span>').appendTo(elt);
-                $('#details-'+identifier+' .'+id_s+' [data-toggle="popover"]').popover();
+                $('#details-'+identifier+' .'+id_s+' [data-toggle="popover"]').popover({container: '#details-'+identifier+' .'+id_s+' [data-toggle="popover"]'});


For the other community usage, there was only one link, so #details-'+identifier+' .'+id_???+' [data-toggle="popover"] was returning only one link. Here we have to links, one for appliances, on for tools, the container of the popover is then both of them.

The fixe would be to distinguish the two link, I think adding an optional css_classes attribute to both to_biosphere_href and to_generic_href would allow to then do

$('<span>' + to_biosphere_href(c[0],caller_s.get_url(),data[0],'app') + ' by appliances, ' + to_biosphere_href(c[1],caller_s.get_url(),data[1],'tool') + ' by tools.' + '</span>').appendTo(elt); $('#details-'+identifier+' .'+id_s+' [data-toggle="popover"].app').popover({container: '#details-'+identifier+' .'+id_s+' [data-toggle="popover"].app'}); $('#details-'+identifier+' .'+id_s+' [data-toggle="popover"].tool').popover({container: '#details-'+identifier+' .'+id_s+' [data-toggle="popover"].tool'});

I couldn't completely follow on the requirement for the new optional .app and .tool classes addition as suggested, and this may require additional changes on to_biosphere_href(...) and to_generic_href(...) functions.
I can separately commit it so that if anything goes different, that individual commit can be improved or reverted.

Just tried the suggested change on adding a class to separate .app and .tool, and those popovers too work very well, without giving any note on one popover opening another one. Thank you very much for these pointed suggestions. I have implemented and pushed it, please review and merge if it looks resolved.

bryan-brancotte · 2021-08-03T09:07:45Z

js/tree-edam-stand-alone.js

@@ -433,7 +433,7 @@ function interactive_edam_browser(){
                        });
                    }
                }
-                $('#details-'+identifier+' .'+id_b+' [data-toggle="popover"]').popover();
+                $('#details-'+identifier+' .'+id_b+' [data-toggle="popover"]').popover({container: '#details-'+identifier+' .'+id_b+' [data-toggle="popover"]'});


It seems to work better !

I think we should wrap this "patch" in a function call, indeed it is error prone to have to copy past the selector. We wrapping we would then do

build_popover('#details-'+identifier+' .'+id_b+' [data-toggle="popover"]');

which would call

function build_popover(selector){ $(selector).popover({container: selector}); }

Yes, doing so would actually reduce the repetitive selector part in code, and will make it cleaner. This would definitely be better than the existing change.

I somehow missed on making this change for quite long, but better later than never. Thanks for keeping this pull request in the last state, I have added build_popover function with its selector just as suggested.

Uses a sub-function build_popover(<selector>) that selects a given selector and initializes a popover for the given selector with its container binding to the same selector passed. Signed-off-by: Harshit Gupta <[email protected]>

Uses css_classes to add class selector for generic_href as suggested to distinguish .app-liances and .tool-s for separate popover initialization to prevent unexpected popover switch on mouse hover. 'css_classes' has been carefully added as an optional parameter with default blank value on all callers of to_generic_href wrappers. Fix: edamontology#225 Signed-off-by: Harshit Gupta <[email protected]>

Update popover() method to prevent its untimely dismissal

5b08114

matuskalas requested review from bryan-brancotte and HagerDakroury August 1, 2021 22:26

bryan-brancotte requested changes Aug 2, 2021

View reviewed changes

Git-Harshit added 2 commits August 2, 2021 18:02

Update popver container with enhanced element selection

54b0abf

Merge branch 'main' into main

089b9b5

bryan-brancotte requested changes Aug 3, 2021

View reviewed changes

Git-Harshit added 2 commits April 11, 2024 12:16

Add build_popover() for popover as suggested

4d12490

Uses a sub-function build_popover(<selector>) that selects a given selector and initializes a popover for the given selector with its container binding to the same selector passed. Signed-off-by: Harshit Gupta <[email protected]>

Git-Harshit requested a review from bryan-brancotte April 21, 2024 06:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update popover() method to prevent its untimely dismissal #227

Update popover() method to prevent its untimely dismissal #227

Git-Harshit commented Aug 1, 2021

bryan-brancotte left a comment

bryan-brancotte Aug 2, 2021

bryan-brancotte commented Aug 2, 2021 •

edited

Loading

Git-Harshit commented Aug 2, 2021 •

edited

Loading

bryan-brancotte left a comment

bryan-brancotte Aug 3, 2021

Git-Harshit Apr 11, 2024

Git-Harshit Apr 11, 2024

bryan-brancotte Aug 3, 2021

Git-Harshit Aug 7, 2021

Git-Harshit Apr 11, 2024

Update popover() method to prevent its untimely dismissal #227

Are you sure you want to change the base?

Update popover() method to prevent its untimely dismissal #227

Conversation

Git-Harshit commented Aug 1, 2021

Checklist

Issue

Details

bryan-brancotte left a comment

Choose a reason for hiding this comment

bryan-brancotte Aug 2, 2021

Choose a reason for hiding this comment

bryan-brancotte commented Aug 2, 2021 • edited Loading

Git-Harshit commented Aug 2, 2021 • edited Loading

bryan-brancotte left a comment

Choose a reason for hiding this comment

bryan-brancotte Aug 3, 2021

Choose a reason for hiding this comment

Git-Harshit Apr 11, 2024

Choose a reason for hiding this comment

Git-Harshit Apr 11, 2024

Choose a reason for hiding this comment

bryan-brancotte Aug 3, 2021

Choose a reason for hiding this comment

Git-Harshit Aug 7, 2021

Choose a reason for hiding this comment

Git-Harshit Apr 11, 2024

Choose a reason for hiding this comment

bryan-brancotte commented Aug 2, 2021 •

edited

Loading

Git-Harshit commented Aug 2, 2021 •

edited

Loading