Skip to content
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

Implemented Bookmark Functionality. #195

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
21 changes: 21 additions & 0 deletions css/index.css
Original file line number Diff line number Diff line change
Expand Up @@ -215,4 +215,25 @@ body {
.flex-column{
flex-direction: column;
}
#bookClick{
Copy link
Member

Choose a reason for hiding this comment

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

The aspect of the bookClick should be reworked. I woulsd sugest smaller and more un the aspect of what is currently done for "previously seen" entries. Also the aspect should allow to un-bookmark an item.

width: 100%;
margin: 8px 8px;
border-radius: 100px;
font-size: 18px;
align-items: center;
box-sizing: border-box;
max-height: none;
min-height: 48px;
padding: 0 17px;
position: relative;
border-color: black;
}
.bookmarks {
column-count: 2;
}
@media screen and (max-width: 600px) {
.bookmarks {
column-count: 1;
}
}
/*end of file*/
2 changes: 1 addition & 1 deletion index.html
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ <h1 id="pageTitle">EDAM ontology</h1>
</div>
</div>
<div class="content row" id="main">
<div class="col-xs-12 col-lg-8">
<div class="col-xs-12 col-lg-8 parentContainer">
<div class="panel panel-default" id="tree-and-controls">
<div class="loader-wrapper">
<i class="fa fa-spinner fa-spin fa-10x"></i>
Expand Down
24 changes: 24 additions & 0 deletions js/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,30 @@ window.onload = function() {
}else{
browser.current_branch(branch);
}
let keys = Object.keys(sessionStorage);
if (keys != "IsThisFirstTime_Log_From_LiveServer") {
Copy link
Member

@bryan-brancotte bryan-brancotte May 19, 2021

Choose a reason for hiding this comment

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

I removed all IsThisFirstTime_Log_From_LiveServer and the code still works, why adding it ? if it works without it, It generally means that we do not need it (unless it prevents error in the specific scenario)

if ($(".bookmarks").length == 0) {
$(".parentContainer").append("<div class='bookmarks'></div>");
Copy link
Member

Choose a reason for hiding this comment

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

Instead of dynamically adding it, add it in index.html where you want. Also consider adding <div id='bookmarks'></div> i.e with an ID as there will be only one bookmark container

}
for (let key of keys) {
if (key != "IsThisFirstTime_Log_From_LiveServer") {
$(".bookmarks").append(
Copy link
Member

Choose a reason for hiding this comment

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

will become $("#bookmarks")

`<button id="bookClick">${key}</button>`
Copy link
Member

Choose a reason for hiding this comment

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

Element ID should/must be unique, we indeed can create multiple element with the same id, but it can have strange behavior. Use classes here : ${key}`

https://www.w3schools.com/html/html_id.asp#:~:text=The%20id%20attribute%20specifies%20a,HTML%20element.

);
}
}

const buttons = document.querySelectorAll("#bookClick");
for (const button of buttons) {
button.addEventListener("click", function () {
browser.interactive_tree().cmd().clearSelectedElements(false);
browser
.interactive_tree()
.cmd()
.selectElement(`${sessionStorage.getItem(button.innerHTML)}`, true);
Copy link
Member

Choose a reason for hiding this comment

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

I'm also new to sessionStorage vs localStorage, you should read for example https://krishankantsinghal.medium.com/local-storage-vs-session-storage-vs-cookie-22655ff75a8 In our case we want to data to be saved after the tab is closed, so we should use local storage.

});
}
}
$("input[name='show-detail']").prop("checked" , (localStorage.getItem("show-detail")||"true") == "true");
$("input[name='show-community-usage']").prop("checked" , (localStorage.getItem("show-community-usage")||"false") == "true");
};
31 changes: 31 additions & 0 deletions js/tree-edam-stand-alone.js
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ function interactive_edam_browser(){
details += '<span>';
details += '<a title="edit this term" type="button" class="btn btn-default btn-xs pull-right" target="_blank" href="edit.html?term='+uri+'&branch='+current_branch+'"><span class="glyphicon glyphicon-pencil"></span></a>';
details += '<a title="add a child to this term" type="button" class="btn btn-default btn-xs pull-right" target="_blank" href="edit.html?parent='+uri+'&branch='+current_branch+'"><span class="glyphicon glyphicon-plus"></span></a>';
details += `<a title="bookmark this term" type="button" class="btn btn-default btn-xs pull-right" id="bookmark" target="_blank"><span class="glyphicon glyphicon-bookmark"></span></a>`;
Copy link
Member

Choose a reason for hiding this comment

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

The status of bookmarked or not should be rendered. Maybe with a gold color, or maybe by toggling between
glyphicon glyphicon-star and glyphicon glyphicon-star-empty

details += '</span>';
details += '</h4>';
details += '</div>';
Expand All @@ -232,6 +233,9 @@ function interactive_edam_browser(){
details += '</div>';
details=$(details);
details.find(".term-name-heading").text(d.data.text);
details.find("#bookmark").click(function () {
bookmark(d.data.text, d.data.data.uri);
Copy link
Member

Choose a reason for hiding this comment

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

the method bookmark ask for the branch, not the text of the node.

});
return details;
}

Expand Down Expand Up @@ -754,3 +758,30 @@ function toggleFullscreen(){
$('#go-fullscreen').show();
}
}
function bookmark(branch, uri) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this method should be reworked, we should not store independently each bookmark, but an array/list/dict of all bookmarked elements for the current branch.

Also, adding a third attribut indicating if we want to add or remove the bookmark would be useful.

sessionStorage.setItem(branch, uri);
let keys = Object.keys(sessionStorage);
Copy link
Member

Choose a reason for hiding this comment

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

Sorting them could also be useful.

for (const key of keys) {
$("#bookClick").remove();
Copy link
Member

Choose a reason for hiding this comment

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

Removing them all to re-add them is not very elegant, but as we don't not have hundreds of them it is no big deal

}
for (const key of keys) {
if (key != "IsThisFirstTime_Log_From_LiveServer") {
if ($(".bookmarks").length == 0) {
$(".parentContainer").append("<div class='bookmarks'></div>");
}
$(".bookmarks").append(
`<button id="bookClick">${key}</button>`
);
}
}
const buttons = document.querySelectorAll("#bookClick");
for (const button of buttons) {
button.addEventListener("click", function () {
browser.interactive_tree().cmd().clearSelectedElements(false);
browser
.interactive_tree()
.cmd()
.selectElement(`${sessionStorage.getItem(button.innerHTML)}`, true);
});
}
}