Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add anchors to Markdown headers #2513
Add anchors to Markdown headers #2513
Changes from 9 commits
2a3e56e
d92cf59
778b4d7
427757e
80fedfc
c41e0ef
3fcf660
bc87c7d
3b64e68
b024e59
f813427
ece7d59
b050b15
c9c790f
bfff598
278e0b3
c82185d
a267646
a243dbb
8027b70
dd302c4
3f7d3f7
59f8a4c
3c3308b
ac5beeb
8e52a07
62469c9
5ae0bee
e64aba3
7339f8f
19974fd
e5285fd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I don't think this would work with cloud. I assume we will want the hash to be in the parent window url bar but the app is in a iframe. Shouldn't block us but is something we should raise to them to support.
Also, if for some reason we have 2 headers with the same anchor, this could scroll through each one until the last one. Not sure if thats desirable? I think this could be good for now but I could see later where we would want to move the scrolling to a global action.
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.
The issue I ran into was that we need to use refs to grab the header elements after they're mounted (so we can use
element.textContent
to get the text to generate the anchor), and there's not really a callback like "when all refs have been received," so there's no natural place to call the global action.I fixed this for now by just keeping track of an
alreadyScrolled
variable and only scrolling the first time. Good concern, we'll definitely circle back to this.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.
Could be id or name on the tag instead? See below for 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.
See #2513 (comment)
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.
I believe anchors are generally populated as the id or name attribute. Multipage app is something we will want to do later and we may need to switch to id later depending how we proceed.
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.
Addressing this comment and your other comment together:
Here all we're doing is finding a way to pass the anchor to the frontend, which eventually intercepts the
data-anchor
in the ReactMarkdown renderer, and never actually prints it out in the<h2>
element.The
<a data-anchor="...">
is because I usedname
at first, but apparently React doesn't support thename
property on<a>
tags. Don't know why.Can you say more about multipage app and how that relates to using id?
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.
Update: I just removed the
<a data-anchor>
altogether as it wasn't doing anything.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.
My first thought for multipage app is using react router and I believe the hash link from react router would scroll to the element matching the ID. Then if we passed the ID here we could just spit out the html as is without doing any modifications.