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

Comments: Add a reference link back to the highlighted section #41

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ibrahima
Copy link

@ibrahima ibrahima commented Aug 2, 2022

The annotation comments/thread messages include a reference to the object that they are annotating, so we can add a link to those comments to point back to that section or annotation. This may not always be applicable for all comments, but when it is available it helps with context. Otherwise it can be hard to decipher what a comment means without that context.

Note that if a comment refers to a deleted document section, there will still be a link that just doesn't go to anything. That seems fine for now.

This relates to #7 where the possibility of including annotation comments was previously discussed.

Screenshot of what it looks like:

image

https://gist.github.com/ibrahima/7fbe8b5f80baa6197406d960d2ab0f32

When you click on a ref link, it highlights the targetted section.

This may not always be applicable, but when it is available it helps with context. Otherwise it can be hard to decipher what a comment means without that context.
@ibrahima ibrahima marked this pull request as ready for review August 2, 2022 23:49
@@ -29,6 +29,10 @@
.message--user {
font-weight: bold;
}

:target {
background-color: yellow;
Copy link
Author

@ibrahima ibrahima Aug 3, 2022

Choose a reason for hiding this comment

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

This makes it so that if you click an anchor link from (e.g. from one of the comments) it will highlight the comment in yellow. We could also use a different color, yellow seemed like a decent default choice, though pure yellow is kind of bright IMO. Some other good options are gold and lightyellow.

ref: https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/color_keywords

I wasn't sure if it made sense to put this here or in the document.css file, but it seemed to me that that file is more for the base Quip CSS and not any additions.

referencedSections = message.annotation.highlight_section_ids.map((sectionId, index) => {
// Not sure if there can be more than one highlight section,
// but if so, we can number the reference links
const suffix =
Copy link
Author

Choose a reason for hiding this comment

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

This formatting is due to prettier, but I did notice that it doesn't seem like there's a line length limit being enforced in this codebase, so let me know if you'd prefer this to be consolidated onto one line. Same goes for the other code in this patch!

message.annotation.highlight_section_ids.length == 1
? ""
: ` ${index + 1}`;
return `<a href="#${sectionId}">ref${suffix}</a>`;
Copy link
Author

Choose a reason for hiding this comment

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

I picked an arbitrary string to use for the reference links, it is either just "ref" or "ref 1", "ref 2" etc if there happen to be multiple values in highlight_section_ids. I don't yet know if this happens in practice but I figured I'd handle the case anyway. Open to any other suggestions on how to name these links.

I also considered just making the timestamp a link but decided against that.

if(text) {
html += `\n<div class="message"><span class="message--user">${message.author_name}${dateStr}<br/></span>${text}</div>`;
html += `\n<div class="message"><span class="message--user">${message.author_name}${dateStr} ${referencedSections}<br/></span>${text}</div>`;
Copy link
Author

Choose a reason for hiding this comment

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

Wondering now if I should remove the space between ${dateStr} and ${highlightedSections} but I think it shouldn't matter since HTML essentially ignores whitespace between tags. But if necessary I can just incorporate it into the highlightedSections variable.

So far I've only seen one case where I had more than one value, but they weren't unique, so it seems helpful to make this unique to avoid extraneous links.
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.

1 participant