-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Conversation
Tests are actually passing, only jslint was failing, just fixed. Edit: never mind, couple tests failing, fixing. |
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 could see a E2E test for this functionality, possibly using snapshots? 🤷 A small unit test for valid anchor types could be helpful.
The rest of my comments are more food for thought. I think this will probably get revamped for multipage apps but in the meantime I think this works.
But we should definitely add documentation for this.
if (newAnchor) { | ||
setAnchor(newAnchor) | ||
if (window.location.hash.slice(1) === newAnchor) { | ||
ref.current.scrollIntoView(true) |
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.
lib/streamlit/elements/markdown.py
Outdated
if anchor is None: | ||
header_proto.body = '## %s' % clean_text(body) | ||
else: | ||
header_proto.body = '<h2 data-anchor="%s">%s</h2>' % (anchor, clean_text(body)) |
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 used name
at first, but apparently React doesn't support the name
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.
Can you say more about multipage app and how that relates to using id?
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.
<> | ||
{anchor && <a data-anchor={anchor} />} | ||
{children} | ||
</> |
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.
Minus the typo and build failure looks good for now.
I am leaning towards some global scroller listening for url changes for the future as we handle url changes. We don't really do anything with the URL besides our experimental query params so I think this can be punted. The current changes should handle the common use case of sending an url with a hash and allowing people to open a page navigated to that anchor.
const hash = window.location.hash.slice(1) | ||
if (hash === (anchor || createAnchorFromText(node.textContent))) { | ||
node.scrollIntoView(true) | ||
alreadyScrolled = true |
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.
Hm so this alreadyScrolled
would prevent scrolling back to the same element when the anchor value changes. But if we somehow changed the hash and then changed it back, I don't believe this will get called / scroll into view. Alternatively if we changed the anchor and changed it back to match the current hash, it wouldn't scroll. Not sure how many scrolling use cases we want to get into with all this scrolling business. Could be something we refine as part of a future task but defer to @asaini.
I'm not sure if this would also prevent us from scrolling to multiple elements? alreadyScrolled
looks to be locally defined so each tag with an anchor would get it's own.
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.
Ok, I think I fixed this by just passing the anchor as the header's id
. The dynamic scrolling handles scrolling on render, and after that, the browser handles everything else for us (clicking on a #link
, manually changing the url, etc.)
I'm not sure if this would also prevent us from scrolling to multiple elements? alreadyScrolled looks to be locally defined so each tag with an anchor would get it's own.
createHeadingWithAnchor()
is only called once to initialize HeaderWithAnchor
, so all instances of <HeaderWithAnchor>
will use that variable stored in the closure. You're right though, it's confusing, I'll just make it global.
…mlit into markdown-header-links
@bh-streamlit : I am able to see the anchor links working in Chrome(v87.0) but not in Safari(v14.0). I am using the following code: import streamlit as st
import pandas as pd
import numpy as np
st.title('blah-one', anchor='anchor-one')
chart_data = pd.DataFrame(
np.random.randn(20, 3),
columns=['a', 'b', 'c'])
st.line_chart(chart_data)
st.header('blah-two', anchor='anchor-two')
chart_data = pd.DataFrame(
np.random.randn(20, 3),
columns=['a', 'b', 'c'])
st.line_chart(chart_data)
st.subheader('blah-three', anchor='anchor-three')
chart_data = pd.DataFrame(
np.random.randn(20, 3),
columns=['a', 'b', 'c'])
st.line_chart(chart_data)
st.subheader('blah-four', anchor='anchor-four')
chart_data = pd.DataFrame(
np.random.randn(20, 3),
columns=['a', 'b', 'c'])
st.line_chart(chart_data) I compiled your branch and on chrome when I try to open Are you able to repro? |
Ok, I investigated. Turns out Safari always refreshes when you press Enter in url bar, unlike Chrome detecting if only the hash was changed and simply scrolling to it. As for it not scrolling after the page, that's because at the moment of scroll, the tables aren't loaded yet, so it "scrolls" to |
Works well in Safari now 👍 |
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.
Only couple of small changes. Otherwise, looks good.
Thanks for an awesome feature!
frontend/src/components/shared/StreamlitMarkdown/StreamlitMarkdown.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/shared/StreamlitMarkdown/StreamlitMarkdown.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/shared/StreamlitMarkdown/StreamlitMarkdown.tsx
Outdated
Show resolved
Hide resolved
@kantuni Made requested changes. |
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.
With few nitpicks
Discussed with Thaigo and we’ll need additional review for this. Please hold on merging |
Thanks again, looks great! |
Got Thiago's approval as well. Just waiting for Karrie's snapshot fix, then merging. |
* develop: Add anchors to Markdown headers (streamlit#2513)
This reverts commit 90e8b0a.
This reverts commit ab26f05.
* develop: ✨ Support "auto" as the new default for st.image's `use_column_width` (streamlit#2635) fix branch name in pr-preview (streamlit#2644) ♻️ Remove "_proto" from "exception_proto.py" (streamlit#2638) ♻️ Remove "_proto" from "media_proto.py" and "iframe_proto.py" (streamlit#2639) Refactor: remove "_proto" from "image_proto.py" (streamlit#2626) correct info on url shortening behavior (streamlit#2576) Set "overflow:visible" on st.expander (streamlit#2611) Revert "Revert "Add anchors to Markdown headers (streamlit#2513)"" Fix file uploader docs + change to getvalue (streamlit#2628) Update change log Update notices Up version to 0.75.0 Revert "Add anchors to Markdown headers (streamlit#2513)" Speed up Cypress tests (streamlit#2600) Remove "beta feature" notice on st.color_picker (streamlit#2625) Deflake multiselect snapshot test by waiting for stale-element (streamlit#2624) Rerender Maybe components when they're first disabled (streamlit#2617) Increase side padding to 5rem when app is in wide mode (streamlit#2613)
…)""" This reverts commit 1ad0509.
* develop: Revert "Revert "Revert "Add anchors to Markdown headers (streamlit#2513)""" (streamlit#2664) ♻️ Remove "_proto" from "data_frame_proto.py" (streamlit#2640)
* develop: ReportContext constructs its own widget_ids_this_run (streamlit#2669) Add types to report_thread.py (streamlit#2665) Take scrollbars into account when computing Dataframe dimensions (streamlit#2622) Revert "Revert "Revert "Add anchors to Markdown headers (streamlit#2513)""" (streamlit#2664) ♻️ Remove "_proto" from "data_frame_proto.py" (streamlit#2640)
* Add anchors to Markdown headers (#2513) * save progress * save work * remove junk code * fix lint errors * clean up code a bit * fix incorrect type * remove debug code * fix heading numbers * make requested changes and clean up HeadingWithAnchor * only scroll once * fix issues * fix jslint * clean things up * handle edge case * missing a void * fix failing test * add e2e tests * add e2e tests * clean up code by using _.once() * fix lint * fix dynamic scrolling * fix typo * make requested changes * run formatting and remove unused import * remove another unused import * make requested changes * add nice link icon to left of headers * make requested design changes (#2726) * Fix anchor styling to work with dark mode (#3035) * fix anchor styling * Revert "fix anchor styling" This reverts commit cb5d7fd. * fix failing cypress tests * fix styling * Add S4A communication for anchor headers (#2982) * add communication with s4a * fix bug * save work * save work * remove console * add tests * remove useless import * fix failing tests * Remove Markdown anchors from sidebar (#3059) * remove markdown anchors from sidebar * fix test * add tests * Fix failing Cypress test for Markdown Anchors (#3077) * see if this fixes things * fix things * fix * fix scrolling * add spacer * save work * remove unneeded changes
* Add anchors to Markdown headers (#2513) * save progress * save work * remove junk code * fix lint errors * clean up code a bit * fix incorrect type * remove debug code * fix heading numbers * make requested changes and clean up HeadingWithAnchor * only scroll once * fix issues * fix jslint * clean things up * handle edge case * missing a void * fix failing test * add e2e tests * add e2e tests * clean up code by using _.once() * fix lint * fix dynamic scrolling * fix typo * make requested changes * run formatting and remove unused import * remove another unused import * make requested changes * add nice link icon to left of headers * make requested design changes (#2726) * Fix anchor styling to work with dark mode (#3035) * fix anchor styling * Revert "fix anchor styling" This reverts commit cb5d7fd. * fix failing cypress tests * fix styling * Add S4A communication for anchor headers (#2982) * add communication with s4a * fix bug * save work * save work * remove console * add tests * remove useless import * fix failing tests * Remove Markdown anchors from sidebar (#3059) * remove markdown anchors from sidebar * fix test * add tests * Fix failing Cypress test for Markdown Anchors (#3077) * see if this fixes things * fix things * fix * fix scrolling * add spacer * save work * remove unneeded changes
Closes #824.
Implements as custom
react-markdown
renderers. Handles:st.title('blah', anchor='some-anchor')
st.header('blah', anchor='some-anchor')
st.subheader('blah', anchor='some-anchor')