Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

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

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5b08114
Update popover() method to prevent its untimely dismissal
Git-Harshit Aug 1, 2021
54b0abf
Update popver container with enhanced element selection
Git-Harshit Aug 2, 2021
089b9b5
Merge branch 'main' into main
Git-Harshit Aug 2, 2021
4d12490
Add build_popover() for popover as suggested
Git-Harshit Apr 11, 2024
913c681
Add classes to separate app and tool selection pop
Git-Harshit Apr 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions js/tree-edam-stand-alone.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ function interactive_edam_browser(){
data_content = "title=\"Some associated elements\""
data_content += "data-toggle=\"popover\""
data_content += "data-placement=\"auto right\""
data_content += "data-trigger=\"hover\""
data_content += "data-trigger=\"hover focus\""
data_content += "data-html=\"true\""
data_content += "data-content=\"<table class=' table table-condensed'>";
var i=0;
Expand Down Expand Up @@ -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"]'});
Copy link
Member

@bryan-brancotte bryan-brancotte Aug 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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});
}

Copy link
Author

@Git-Harshit Git-Harshit Aug 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Git-Harshit Git-Harshit Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
}
var caller_s=biosphere_api().get_for(current_branch, __my_interactive_tree.textAccessor()(d), uri, d);
Expand All @@ -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"]'});
Copy link
Member

@bryan-brancotte bryan-brancotte Aug 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'});

Copy link
Author

@Git-Harshit Git-Harshit Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Git-Harshit Git-Harshit Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
EDAM_Ontology_popover_stable

});
}
var caller_w=bioweb_api().get_for(current_branch, __my_interactive_tree.textAccessor()(d), uri, d);
Expand All @@ -456,7 +456,7 @@ function interactive_edam_browser(){
var elt=$('#details-'+identifier+' .'+id_w);
elt.empty();
$(to_bioweb_href(c,caller_w.get_url(),data)).appendTo(elt);
$('#details-'+identifier+' .'+id_w+' [data-toggle="popover"]').popover();
$('#details-'+identifier+' .'+id_w+' [data-toggle="popover"]').popover({container: '#details-'+identifier+' .'+id_w+' [data-toggle="popover"]'});
});
}
var caller_t=tess_api().get_for(current_branch, __my_interactive_tree.textAccessor()(d), uri, d);
Expand All @@ -466,7 +466,7 @@ function interactive_edam_browser(){
var elt=$('#details-'+identifier+' .'+id_t);
elt.empty();
$(to_tess_href(c,caller_t.get_url(),data)).appendTo(elt);
$('#details-'+identifier+' .'+id_t+' [data-toggle="popover"]').popover();
$('#details-'+identifier+' .'+id_t+' [data-toggle="popover"]').popover({container: '#details-'+identifier+' .'+id_t+' [data-toggle="popover"]'});
});
}
if(uri.startsWith("http://edamontology.org/")){
Expand Down