Refactor VisuallyHidden component to use ChakraUI #7724
Conversation
minimalsm
requested review from
samajammin,
wackerow,
corwintines,
pettinarip and
jmcook1186
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
src/components/VisuallyHidden.tsx
Outdated
| @@ -1,43 +1,18 @@ | |||
| import React from "react" | |||
| import styled from "@emotion/styled" | |||
| import { VisuallyHidden as VisuallyHiddenSpan } from "@chakra-ui/react" | |||
There was a problem hiding this comment.
Alias here as conflicted with component name
There was a problem hiding this comment.
One thing on this, can you call it ChakraVisuallyHidden? since we are calling those aliases with the Chakra prefix in other files
src/components/VisuallyHidden.tsx
Outdated
| z-index: 100000; | ||
| } | ||
| ` | ||
| // Todo: Refactor to from isHidden boolean |
There was a problem hiding this comment.
Obviously out of scope for this refactor, but I disagree with how this component is architected.
If we wrap something as VisuallyHidden, we shouldn't also need to pass a boolean to hide the item we're wrapping.
There was a problem hiding this comment.
&& we might not even need a component for this at all if all we're doing is importing Chakra's primitive 🤔?
There was a problem hiding this comment.
Yes, this one is one of those components that we should remove completely and replace with the Chakra one.
My personal approach to this would be to use this PR to remove it and refactor the usage of this component in the entire codebase in favor of the Chakra one.
I'm fine as well if you want to take this middle ground approach. We could do the refactor later.
There was a problem hiding this comment.
Can do, the only place we use it is in Layout.tsx.
Are we fine with just conditionally rendering, or is there a smarter way to go about it 🤔
{isZenMode ? (
<VisuallyHidden>
<Nav path={path} />
{shouldShowSideNav && <SideNavMobile path={path} />}
</VisuallyHidden>
) : (
<React.Fragment>
<Nav path={path} />
{shouldShowSideNav && <SideNavMobile path={path} />}
</React.Fragment>
)}There was a problem hiding this comment.
Hmm looking into the code, I think it would be worth abstracting the zen mode state in its own component.
<ZenMode>
<Nav />
...
</ZenMode>Inside ZenMode, you will have all of the logic
function ZenMode({children}) {
const [isZenMode, setIsZenMode] = useState(false)
...
if(isZenMode) {
return (
<VisuallyHidden>{children}</VisuallyHidden>
)
}
return chilren
}There was a problem hiding this comment.
Good idea. I will get to this when I have some free evening time this week.
There was a problem hiding this comment.
I think this is easier said than done given the handleZenModeChange in Layout.tsx is passed to a provider. Will do some thinking here today.
There was a problem hiding this comment.
I think we can just import this context though instead of this. Still ways to clean it up.
✅ ethereum-org-website-dev deploy preview ready
|
src/components/VisuallyHidden.tsx
Outdated
| @@ -1,43 +1,18 @@ | |||
| import React from "react" | |||
| import styled from "@emotion/styled" | |||
| import { VisuallyHidden as VisuallyHiddenSpan } from "@chakra-ui/react" | |||
There was a problem hiding this comment.
One thing on this, can you call it ChakraVisuallyHidden? since we are calling those aliases with the Chakra prefix in other files
|
@minimalsm to update this |
src/components/VisuallyHidden.tsx
Outdated
| } | ||
|
|
||
| const VisuallyHidden: React.FC<IProps> = ({ isHidden = false, children }) => | ||
| isHidden ? <Container>{children}</Container> : <>{children}</> | ||
| const VisuallyHidden: React.FC<IProps> = ({ children }) => { |
There was a problem hiding this comment.
I think we need to change the name of this component. This is not a generic component anymore. This is strictly related to zen mode. Would suggest calling this component ZenMode.
There was a problem hiding this comment.
Agree... just pushed a commit for this. I think this should be good to go now.
Description
Migrate VisuallyHidden from styled component to ChakraUI
Related Issue
#6374