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

📚 docs: open details section when linking items on it #42402

Merged
merged 4 commits into from
Jul 23, 2024

Conversation

letiescanciano
Copy link
Contributor

@letiescanciano letiescanciano commented Jul 22, 2024

What

Closes #8754
Hash linking (e.g https://docs.airbyte.com/integrations/sources/stripe#rate-limiting ) to items inside <details> elements does not cause <details> to be opened by default.
These changes improves UX so if the pathname contains a hash that it's inside a <details> element, it will open the element.

This works also for the title of the section (the element just before <details> )

How

  • Implementing a custom component for <details> and setting it to open based on current url.

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

Copy link

vercel bot commented Jul 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 23, 2024 7:28am

Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

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

Had a few comments, up to you whether to address them before merging or in a follow-up PR. Otherwise this looks good to me, nice!

@@ -0,0 +1,41 @@
.details {
--docusaurus-details-decoration-color: var(--ifm-color-info-dark);
--docusaurus-details-transition: transform var(--ifm-transition-fast) ease;
Copy link
Contributor

Choose a reason for hiding this comment

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

One different I noticed between this and the original docusaurus details is that the docusaurus one would animate a "slide down" of the content when opening, and "slide up" when closing, but this custom one just immediately opens or closes the full content.

I don't think this is a huge deal if it is hard to get working right, but just wanted to point it out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to copy the behaviour but can't make it work. I'll add this in a new issue as a nice-to-have to unblock this one :)

@@ -21,4 +21,5 @@ export default {
SpecSchema,
PyAirbyteExample,
ProductInformation,
Details,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised that this is all that is needed to replace the default details component!

Is that something specific to docusaurus, such that you just need to have a component named the same (but with a capitalized first letter)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think is actually a Markdown thing. It seems that both <Details> and <details> will render become <details>.

Eg:

will be rendered automatically as

This was easy to spot on docusaurus by logging MDXComponents from "@theme-original/MDXComponents"; and our CustomComponents export.
I'm actually overwriting the definition of <Details> by using the same name, which it turns out it overwrites both.

I think this is useful in this case, because this is the default behaviour that we want for this component, but let me know if you would prefer to have a separate component!

https://www.loom.com/share/f1842ecd722a43bdb3e878ac0888949b?sid=a1ed795d-70b5-499c-ad6c-ea12dc7fbb27

Copy link
Contributor

Choose a reason for hiding this comment

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

No this makes sense, I am pleasantly surprised that the overriding is this easy to configure 👍

import React from "react";
import styles from "./Details.module.css";

export const Details = ({ className, ...rest }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried testing with a nested <details> tag.

While opening a new tab with a hash of a nested header worked, clicking on another header and then clicking on the header in that nested <details> section causes the entire parent details section to be collapsed. See recording below:

Screen.Recording.2024-07-22.at.10.33.45.AM.mov

Ideally this would work as well, but doesn't necessarily need to block this merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! I'll create a new issue and prioritize it with Natalie :)

@letiescanciano letiescanciano merged commit 1d8e5b2 into master Jul 23, 2024
31 checks passed
@letiescanciano letiescanciano deleted the leti/improve-hyperlinks-in-details-sections branch July 23, 2024 08:34
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.

2 participants