Add auto-linked headers on glossary page [Fixes #12393] #12455
Conversation
checkomkar
requested review from
wackerow,
corwintines,
pettinarip,
minimalsm and
nhsz
as code owners
WalkthroughThe update to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/components/Glossary/GlossaryDefinition/index.tsx (2 hunks)
Additional comments: 3
src/components/Glossary/GlossaryDefinition/index.tsx (3)
- 1-3: The imports have been updated to include
useStatefrom "react", and additional components from "@chakra-ui/react" and "react-icons/vsc". Ensure that these new dependencies are properly documented and their versions are compatible with the project's current setup.- 27-27: Introduced state management using
useStateto control the visibility of the link icon. This is a straightforward and effective approach for handling hover interactions. However, consider adding a brief comment explaining the purpose ofshowLinkstate for future maintainability.- 35-47: The styling for the link and link icon is defined inline. While this is acceptable for small components, consider moving these styles to a separate file or using a theming approach if the project's scale increases. This would improve maintainability and consistency across the project.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/components/Glossary/GlossaryDefinition/index.tsx (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/components/Glossary/GlossaryDefinition/index.tsx
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/components/Glossary/GlossaryDefinition/index.tsx (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/components/Glossary/GlossaryDefinition/index.tsx
wackerow
changed the title
There was a problem hiding this comment.
Hey @checkomkar, thanks for this!
Couple things... we seem to be recreating a lot of logic and styling that has already been built elsewhere...
- We should use the same icon as the others (
CiLinkfromreact-icons/ci), and the size should match - The word itself should not be clickable... the link icon should appear while hovering the word, but only the icon should contain the link itself
- Needs a
scrollMarginTopso the header isn't hidden by the nav bar when navigating to it
This is all implemented inside MdComponents for the standard markdown headers, ie Heading3. These utilize a reusable IdAnchor component (also in that file) that handles the link and icon:
This component is then placed inside a header, along with {...commonHeadingProps(id)} to get the props needed for styling, and to assign the appropriate ID.
I'd ask that we take another look here and try to simplify, reusing components where possible and removing the redundant logic. Let us know if you're having issues!
(Last note, the files need to be linted/formatted before they will build correctly... in particular, the imports at top need to follow a certain order... VS Code formatter can do this automatically, but if you're not sure how to set this up we can handle fixing that up when ready)
Thank you @wackerow for the detailed review. Will make the changes as suggested. Also with regards to linting, I tried through VS code somehow it's not working, the import order doesn't seem to sort. Is there any extension that I am supposed to use or any lint command? |
@checkomkar Try |
wackerow
added
Status: Blocked 🛑
Hey @wackerow I tried importing and reusing the components as you suggested, but it was causing cyclic import error so I just copied the |
… fix/issue-12393 merge dev to local branch
@checkomkar To use the existing export const IdAnchor = ({ id }: { id?: string }) => {Then we can import this inside import { IdAnchor } from "@/components/MdComponents"We can then remove the entire |
|
Just checking in, @checkomkar let me know if you want a hand getting this over the line |
I get this cyclic import error whenever I am trying to import
|
|
@checkomkar // This:
import { IdAnchor } from "@/components/MdComponents"
// Not this:
import IdAnchor from "@/components/MdComponents"That file had a You should be able to use the |
|
Checking in... Any luck @checkomkar? Happy to try and get this over the line if you're not available |
prevents circular reference issues from inside MdComponents
There was a problem hiding this comment.
Just extracted the IdAnchor component as it's own file component to avoid any circular reference issues... looks good! Pulling in, thanks a lot @checkomkar!
|
@all-contributors please add @checkomkar for code |
|
I've put up a pull request to add @checkomkar! 🎉 |
Fixes [#12393]
Summary by CodeRabbit
Summary by CodeRabbit