Skip to content

Fix regression in visibility of st.code's copy-to-clipboard button #6498

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.

Sign up for GitHub

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

Jump to bottom
Merged
lukasmasuch merged 8 commits into from
Apr 21, 2023
Merged

Fix regression in visibility of st.code's copy-to-clipboard button #6498

lukasmasuch merged 8 commits into from
Apr 21, 2023

Conversation

@snehankekre
Copy link
Contributor

@snehankekre snehankekre commented Apr 17, 2023 •
edited
Loading

📚 Context

Fixes #6490. PR #6042, while refactoring st.code to be markdown independent, likely introduced a regression in st.code, where the copy button does not appear when you hover over code blocks. This issue was caused by missing CSS hover rules and the way the hover effect was implemented. The button is now restored and appears on hover as expected.

  • What kind of change does this PR introduce?

    • Bugfix
    • Feature
    • Refactoring
    • Other, please describe:

🧠 Description of Changes

  • Modified StreamlitSyntaxHighlighter.tsx to include the StyledCopyButtonContainer and StyledCodeBlock components, which were previously missing.

  • Updated the StyledCopyButtonContainer in styled-components.ts to set the correct opacity, width, and pointerEvents properties, ensuring that the copy button is correctly rendered for st.code.

  • Added the necessary hover and active states to StyledCopyButton in styled-components.ts.

  • Removed the redundant StyledCopyButtonContainer import and usage from StreamlitMarkdown.tsx, as it's now integrated into StreamlitSyntaxHighlighter.

    • This is a breaking API change
    • This is a visible (user-facing) change

Revised:

image

Current:

image

🧪 Testing Done

  • Screenshots included
  • Added/Updated unit tests
  • Added/Updated e2e tests

🌐 References

Does this depend on other work, documents, or tickets?

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@snehankekre snehankekre added the security-assessment-completed Security assessment has been completed for PR label Apr 17, 2023
@snehankekre
Copy link
Contributor Author

snehankekre commented Apr 17, 2023

TODO: write e2e test such that Cypress hovers over the code block (so the copy button is visible) and then takes snapshots.

@sfc-gh-kbregula
Copy link
Contributor

sfc-gh-kbregula commented Apr 17, 2023

We have two usages of the StreamlitSyntaxHighlighter component.

streamlit/frontend/src/components/core/Block/ElementNodeRenderer.tsx

Lines 640 to 645 in 4289ea8

<StreamlitSyntaxHighlighter
language={codeProto.language}
showLineNumbers={codeProto.showLineNumbers}
>
{codeProto.codeText}
</StreamlitSyntaxHighlighter>

streamlit/frontend/src/components/shared/StreamlitMarkdown/StreamlitMarkdown.tsx

Lines 273 to 275 in 4289ea8

<StreamlitSyntaxHighlighter language={language} showLineNumbers={false}>
{codeText}
</StreamlitSyntaxHighlighter>

In both cases we need these buttons, so I think we can move this code inside the StreamlitSyntaxHighlighter component to avoid code duplication.

@snehankekre
Move copy button rendering and its container styles from ElementNodeR…
42a1a3a 
…enderer.tsx and StreamlitMarkdown.tsx to StreamlitSyntaxHighlighter.tsx
@snehankekre
Copy link
Contributor Author

snehankekre commented Apr 18, 2023

In both cases we need these buttons, so I think we can move this code inside the StreamlitSyntaxHighlighter component to avoid code duplication.

Thanks for that insight, @sfc-gh-kbregula! Fixed on 42a1a3a.

There was also a bug in my initial fix: for st.code, the copy button appears only when you hover over the extreme right side of the code block, whereas with st.markdown, it appears when you hover anywhere in the code block. Also fixed by 42a1a3a:

  • Moved the copy button rendering and its container styles from ElementNodeRenderer.tsx and StreamlitMarkdown.tsx to StreamlitSyntaxHighlighter.tsx. This centralizes the copy button handling in one place, removing code duplication.
  • Updated StyledCopyButtonContainer and StyledCopyButton styles in styled-components.ts for better positioning and visibility. The StyledCopyButtonContainer now has pointerEvents: "none" and the StyledCopyButton has pointerEvents: "auto" to ensure proper click handling.
  • Updated the StreamlitSyntaxHighlighter component to render the copy button and its container when the provided children prop is a string. This ensures the copy button is correctly displayed for both st.code and st.markdown.

@sfc-gh-kbregula
Copy link
Contributor

sfc-gh-kbregula commented Apr 18, 2023

@snehankekre Do you need help with tests? I'll be happy to deal with it.

@snehankekre
Copy link
Contributor Author

snehankekre commented Apr 18, 2023

@sfc-gh-kbregula that would be awesome, thank you! 🙏 Reading the Cypress docs on how to hover and then write tests would take me a couple hours.. time I could instead spend on st.connection docs if possible.

@snehankekre snehankekre changed the title Fix regression in st.code copy button and update ElementNodeRenderer Fix regression in visibility of st.code's copy-to-clipboard button Apr 18, 2023
@sfc-gh-kbregula
Copy link
Contributor

sfc-gh-kbregula commented Apr 18, 2023

@snehankekre I added E2E tests.

@snehankekre
Copy link
Contributor Author

snehankekre commented Apr 19, 2023

LGTM, thanks Kamil!

@sfc-gh-kbregula
Copy link
Contributor

sfc-gh-kbregula commented Apr 19, 2023

@snehankekre Do you need any more help with this PR? Is there anything left to do?

@snehankekre
Copy link
Contributor Author

snehankekre commented Apr 19, 2023

All that's left for this PR is code review.

@snehankekre snehankekre requested a review from mayagbarnes April 19, 2023 14:08
@snehankekre snehankekre added type:regression This bug is a regression from previous behavior feature:st.code Related to the `st.code` element labels Apr 21, 2023
@lukasmasuch lukasmasuch merged commit 9d5c5e3 into develop Apr 21, 2023
tconkling added a commit to tconkling/streamlit that referenced this pull request Apr 24, 2023
@tconkling
Merge branch 'develop' into tim/FrontendAppLibSplit
dea1d97 
* develop:
  Removing viz-1.8.0.min.js (streamlit#6520)
  st.experimental_connection: The big merge (streamlit#6487)
  Add additional attributions (streamlit#6536)
  Fix code block font change (streamlit#6535)
  Fully remove email from new session message (streamlit#6516)
  Clarify what telemetry data our backend stores (streamlit#6463)
  Update emojis to latest state (streamlit#6532)
  Add support for pandas 2.0 (streamlit#6507)
  Fix regression in visibility of `st.code`'s copy-to-clipboard button (streamlit#6498)
  Fix E2E image (streamlit#6524)
  Add tenacity as dependency (streamlit#6529)
@tdatIT tdatIT mentioned this pull request May 3, 2023
Closed
@vdonato vdonato deleted the st-code-copy-button branch November 2, 2023 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukasmasuch lukasmasuch Awaiting requested review from lukasmasuch

@mayagbarnes mayagbarnes Awaiting requested review from mayagbarnes

1 more reviewer

@sfc-gh-kbregula sfc-gh-kbregula sfc-gh-kbregula approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

feature:st.code Related to the `st.code` element security-assessment-completed Security assessment has been completed for PR type:regression This bug is a regression from previous behavior

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The copy-to-clipboard button for st.code does not exist anymore

4 participants

@snehankekre @sfc-gh-kbregula @lukasmasuch