-
Notifications
You must be signed in to change notification settings - Fork 39
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
Surfaces the titles to the webUI and associates with the respective JSON and links for #178 #570
Conversation
…SON and links for #178
Codecov Report
@@ Coverage Diff @@
## master #570 +/- ##
==========================================
- Coverage 23.88% 23.84% -0.05%
==========================================
Files 6 6
Lines 1189 1191 +2
Branches 178 179 +1
==========================================
Hits 284 284
- Misses 885 887 +2
Partials 20 20
Continue to review full report at Codecov.
|
ipwb/assets/webui.js
Outdated
@@ -32,6 +32,9 @@ function addURIListToDOM () { | |||
|
|||
li.setAttribute('data-mime', memento['mime']) | |||
li.setAttribute('data-status', memento['status']) | |||
if ('title' in memento) { | |||
li.setAttribute('title', memento['title']) |
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.
If we are planning to merge #566 (after title inclusion) then this change in the webui.js
is not desired as it will cause unnecessary conflict. Making the title data available for usage should be good enough for this PR.
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.
@ibnesayeed This association is an effort to get #566 merged. #566 currently does not deal with titles. By associating it in the UI with the li
, we can work toward including it in the listing.
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.
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.
@ibnesayeed Rm'd in 8b866e6. Once this PR is merged, we can prettify #566 a bit more before merging it in.
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!
They are current associated with the
li
'stitle
DOM attribute. We can change this in the future but having the data associated with the memento listing is a first step toward #178.