-
-
Notifications
You must be signed in to change notification settings - Fork 9k
docs: new APITable comp to render large tables #5891
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
Conversation
Size Change: +431 B (0%) Total Size: 882 kB
ℹ️ View Unchanged
|
✔️ [V2] 🔨 Explore the source changes: 0ebe5dd 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/618b28cc78de4b0008b2b0d7 😎 Browse the preview: https://deploy-preview-5891--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5891--docusaurus-2.netlify.app/ |
|
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.
Thanks, LGTM and nice idea.
We can give it a try and improve later if bugs are found
const highlightedRow = useRef<HTMLTableRowElement>(null); | ||
const rows = Children.map( | ||
tbody.props.children, | ||
(row: ReactElement<ComponentProps<'tr'>>) => { |
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 extract an <APITableRow>
component, and encapsulate the highlighting logic inside it?
Also use of useHistory
rather than DOM APIs that do not trigger re-renders on route changes (in this case it might not produce any weird side effects but is still a bad practice to access window during render execution)
My React has always been hacky especially for these comps that hook heavily into the context...🌚 |
LGTM thanks 👍 It's a bit unnatural to use a conditional ref assignation and forwardRef (I would have used useEffect inside the row) but as it's local and may be more optimized, why not |
I don't really know... I did intend to make one table responsible for only highlighting one row, hence only one ref being handled, but we can see if in the future we want to use query params and highlight multiple rows (like GitHub lines) |
Motivation
When giving instructions on how to enable a particular option, it usually is quite awkward to give a link to the entire API table instead of pointing them to a specific row.
This PR introduces a new
APITable
on the website which allows us to direct to a specific row through anchor links. When visitors click on that link, the respective entry will also be highlighted.Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
https://deploy-preview-5891--docusaurus-2.netlify.app/docs/api/plugins/@docusaurus/plugin-content-blog#archiveBasePath