View comments (In Ideal Conditions) #9
Conversation
…cross element boundaries
There was a problem hiding this comment.
Garrett, I don't expect you to continue working on this since the quarter's over, but thanks for making the PR for someone to continue this.
Rest of the team, I think just whoever the JIRA transfers to can take this branch and keep working off of it/fixing issues. Does that sound reasonable? Anyone interested in it?
| if (comments.length === 1) { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Why exactly 1? What if it's the empty string?
|
|
||
| /// Currently reads from text file - change to read from database later | ||
| /// May require adjustment for formatting | ||
| const getComments = (): Promise<NTRComment[]> => { |
There was a problem hiding this comment.
Good use of return type - a Promise is exactly what we need here.
| } | ||
|
|
||
| /// Temporary function for interpreting the demo comments | ||
| const interpComments = (comments: string): NTRComment[] => { |
There was a problem hiding this comment.
This is a somewhat messy function that could be greatly simplified by just storing comments as a json file, but since it's temporary I'll let it slide.
| /// Currently reads from text file - change to read from database later | ||
| /// May require adjustment for formatting | ||
| const getComments = (): Promise<NTRComment[]> => { | ||
| return fetch('testComments.txt') |
There was a problem hiding this comment.
No .catch() block, and error cases don't seem to be handled when this is called either. Please add that to at least one place, or create a JIRA for it (and post the link here)
| document.addEventListener("selectionchange", () => { | ||
| // This is all a bunch of temporary code showing what can be done with selections and ranges. | ||
| // Because of this, the style is pretty awful. We should discuss exactly what we want to do with | ||
| // selections and what format we want the data in before we proceed too much further. | ||
| const selection: Selection | null = document.getSelection(); | ||
| if (selection == null || selection.rangeCount === 0) { | ||
| if (lastSelected) { | ||
| lastSelected.style.backgroundColor = lastSelectedBackground; | ||
| } | ||
| return; | ||
| } | ||
| const range: Range = selection.getRangeAt(0); | ||
| /* | ||
| Do things with selection here | ||
| https://developer.mozilla.org/en-US/docs/Web/API/Selection | ||
| https://developer.mozilla.org/en-US/docs/Web/API/Range | ||
| */ | ||
| const node: Node | null = range.startContainer.parentNode; | ||
| if (node) { | ||
| if (lastSelected) { | ||
| lastSelected.style.backgroundColor = lastSelectedBackground; | ||
| } | ||
| lastSelected = (node as HTMLElement); | ||
| lastSelectedBackground = (node as HTMLElement).style.backgroundColor; | ||
| (node as HTMLElement).style.backgroundColor = "red"; | ||
| } | ||
| }); |
There was a problem hiding this comment.
This was temporary example code removed in last merge to main. Please make sure it stays deleted.
| let lastSelectedBackground: string = ""; | ||
|
|
||
| comments.then((com: NTRComment[]) => { | ||
| com.forEach(element => { |
There was a problem hiding this comment.
Can you extract this inner lambda passed to forEach into a variable so it has a name and is accessible elsewhere? Ideally somewhere in comments.ts
| "<span id=comment_text_" + i + " onMouseEnter=\"document.getElementById("comment_" + i + | ||
| "").style.display = "block"\" onMouseLeave=\"document.getElementById("comment_" + i + | ||
| "").style.display = "none"\">" + | ||
| elem.innerHTML.substring(element.startOffset, element.endOffset) + | ||
| "</span><div id=comment_" + i + " style=\"background-color: none; display: none; text-align: center;\">" + | ||
| element.text + "</div>" + | ||
| elem.innerHTML.substring(element.endOffset); |
There was a problem hiding this comment.
This is pretty hard to read - it may be a better approach to add just an empty element with a special id, and then attach a React component for comments to it. Then it'll be much more easily readable, more easily modifiable (epecially with onMouseEnter/onMouseLeave events, but those should just be CSS), and more easily testable.
Basically this just allows you to view a comment if it's just within one text node. You can hover over the highlighted text and it will show the comment text. Pretty simple.
I have some temporary functions in comments.ts, so I'll leave some information here so you guys know what to do with them.
getComments(): this is just the function that retrieves raw comment data. Currently it retrieves it from the text filetestComments.txt(this can also be removed later), but it can be rewritten entirely to retrieve data from a database or something.interpComments(): this function parses raw comment data into anNTRComment. Pretty much depends on how you're storing the raw comment data, so feel free to do what you will with it.There are also some issues with displaying multiple comments, but I'm not sure if it's a problem with these temporary functions or a problem with the actual code for displaying comments.
Oh yeah, and a quick overview of the
NTRComment.text: this is the comment that someone leaves. Just raw text for now, feel free to replace it with something else.nodeID: the ID of the node that the comment is left on. Go for the lowest-level node that contains the whole comment.startOffset&endOffset: pretty self-explanatory. Just the starting and ending offsets in # of characters from the start of the node.