Update feedback components for Chakra #7906
Conversation
wackerow
requested review from
corwintines,
jmcook1186,
minimalsm,
pettinarip and
samajammin
as code owners
✅ ethereum-org-website-dev deploy preview ready
|
pettinarip
left a comment
pettinarip
left a comment
There was a problem hiding this comment.
Nicely done @wackerow left a few comments
src/components/NakedButton.tsx
Outdated
| import React from "react" | ||
| import { Button, ButtonProps } from "@chakra-ui/react" | ||
|
|
||
| const NakedButton: React.FC<ButtonProps> = ({ children, ...props }) => ( |
There was a problem hiding this comment.
Have you check the ghost variant? https://chakra-ui.com/docs/components/button/usage#button-variants
I think we should remove this component and start using <Button variant="ghost">. Any specific style we want to do on top of the default variant styles, we could add them on the Chakra theme definition as we did with the other variants.
Check the variants key in here.
| export interface IProps { | ||
| className?: string | ||
| } | ||
| const FixedDot: React.FC<FixedDotProps> = ({ |
There was a problem hiding this comment.
Just want to mention that there is a Chakra component called IconButton https://chakra-ui.com/docs/components/icon-button that perhaps could be a replacement for this. Not saying that this is in the scope of this PR but perhaps it is a better solution in the long term.
There was a problem hiding this comment.
Yes this component has more changes to come in a separate PR, thanks for noting!
src/components/Layout.tsx
Outdated
| <Footer /> | ||
| </VisuallyHidden> | ||
| <FeedbackWidget /> | ||
| <FeedbackWidget location={path} /> |
There was a problem hiding this comment.
One common practice to deal with this is to do
<FeedbackWidget key={path} />that way, the component is going to remount everytime the path change and you will not have to do any manual reset in the inner component logic.
Co-Authored-By: Pablo Pettinari <[email protected]>
pettinarip
left a comment
pettinarip
left a comment
There was a problem hiding this comment.
Nice! I think we can still remove some styles from our custom icon variant and fix the markdown semantics on the Languages button.
| className={className} | ||
| <Flex | ||
| id="modal" | ||
| ref={containerRef} |
There was a problem hiding this comment.
I think we should aim to use the Chakra Popover in this component. https://chakra-ui.com/docs/components/popover we will have better a11y and will abstract us for a bunch of things we are doing here (close icon, modal styles, focus trap, etc).
Not saying this should be in the scope of this PR but I'll create a new issue to tackle that after we merge this.
github-actions
bot
added
the
translation 🌍
Co-authored-by: Pablo Pettinari <[email protected]>
pettinarip
left a comment
pettinarip
left a comment
There was a problem hiding this comment.
Awesome! 🚀
I've just merged dev to fix merge conflicts 👍🏼
Description
Related Issue
#6374