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

Implement "Add to Bookmarks" feature #2440

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

Conversation

dlaliberte
Copy link
Collaborator

This PR implements the "Add to Bookmarks" feature, though it is more of a prototype or proof of concept since I expect many things should be changed.

This addresses 2282

When hovering over a header, the "Add bookmark" icon is displayed above the header and aligned to the left. Hovering over that icon shows a tooltip saying "Add bookmark".

image

Clicking the icon adds a bookmarks section to the table of contents and adds a link to the header to an unordered list, which looks like this:

image

Deficiencies:

  • The text shown in the bookmark is the id of the header, but should probably be the full text.
  • There is no way to remove a bookmark once it has been added.
  • There is no memory of bookmarks if the user refreshes the page.

This is implemented as a change only to the bikeshed/boilerplate.py. Would that be enough? How do we deal with custom boilerplates?

@tabatkins
Copy link
Collaborator

Nice!

The text shown in the bookmark is the id of the header, but should probably be the full text.

Agreed. Just pulling the .textContent of the span.content element should work.

There is no way to remove a bookmark once it has been added.

Not an MVP feature but shouldn't be difficult either, just drop a focusable delete icon at the end of the bookmark text.

There is no memory of bookmarks if the user refreshes the page.

Probably not an issue, since the ECMAScript spec doesn't do this either. If we did add this we'd want to add a little more power, too, like the ability to clear all bookmarks easily. Fine with leaving this out for now.

This is implemented as a change only to the bikeshed/boilerplate.py. Would that be enough? How do we deal with custom boilerplates?

Yes, this is fine. Boilerplate files are just some HTML; they don't have anything to do with the Python-generated boilerplate stuff here.

I'll leave some inline review comments on the actual code.

bikeshed/boilerplate.py Outdated Show resolved Hide resolved
bikeshed/boilerplate.py Outdated Show resolved Hide resolved
bikeshed/boilerplate.py Outdated Show resolved Hide resolved
@dlaliberte
Copy link
Collaborator Author

There is no way to remove a bookmark once it has been added.

Not an MVP feature but shouldn't be difficult either, just drop a focusable delete icon at the end of the bookmark text.

Well, I added the ability to remove bookmarks, but I also have to suppress the default click action when clicking the tooltip of the bookmark since it is a link. Not sure that is working yet. Since my last changes, I can't view anything on my cloudtop.

There are also some CI/lint errors I don't understand.

@tabatkins
Copy link
Collaborator

There are also some CI/lint errors I don't understand.

Cool, I'll look into these in a bit.

@tabatkins
Copy link
Collaborator

Ah, it's a black complaint - I use black to autoformat the code. No worries, I can reformat before merging.

@dlaliberte
Copy link
Collaborator Author

Well, I added the ability to remove bookmarks, but I also have to suppress the default click action when clicking the tooltip of the bookmark since it is a link. Not sure that is working yet. Since my last changes, I can't view anything on my cloudtop.

After two days of trying to get my remote cloudtop working as it was, with no idea why it suddenly stopped working, and two techstop chat sessions that helped narrow down the problem to "Forbidden" access to the remote server, I still don't know why, or how to fix it, so I haven't done any more work on this issue. Maybe it would be best to just disable the "Remove bookmark" feature for now, which is easy enough.

@dlaliberte
Copy link
Collaborator Author

Ah, it's a black complaint - I use black to autoformat the code. No worries, I can reformat before merging.

I managed to install the 'black' formatter extension for vscode, and reformatted the document, which resulted in a few changes that now pass the lint tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants