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.

Sign up for GitHub

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

Refactor VisuallyHidden component to use ChakraUI #7724

Merged
merged 3 commits into from
Nov 14, 2022
Merged

Refactor VisuallyHidden component to use ChakraUI #7724

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8ea89c5
Refactor VisuallyHidden component to use ChakraUI
minimalsm Sep 6, 2022
df2f9b1
change requests and cleanup
corwintines Nov 3, 2022
bf32b5d
update component name to ZenMode
wackerow Nov 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 7 additions & 32 deletions src/components/VisuallyHidden.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,43 +1,18 @@
import React from "react"
import styled from "@emotion/styled"
import { VisuallyHidden as VisuallyHiddenSpan } from "@chakra-ui/react"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alias here as conflicted with component name

Copy link
Member

Choose a reason for hiding this comment

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

One thing on this, can you call it ChakraVisuallyHidden? since we are calling those aliases with the Chakra prefix in other files


// "Accessibility/SEO Friendly CSS Hiding"
// Source: https://css-tricks.com/snippets/css/accessibilityseo-friendly-css-hiding/
const Container = styled.span`
border: 0;
clip: rect(1px, 1px, 1px, 1px);
clip-path: inset(50%);
height: 1px;
margin: -1px;
overflow: hidden;
padding: 0;
position: absolute;
width: 1px;
word-wrap: normal !important;
&:focus {
background-color: #eeeeee;
clip: auto !important;
clip-path: none;
color: #444444;
display: block;
font-size: 1em;
height: auto;
left: 5px;
line-height: normal;
padding: 15px 23px 14px;
text-decoration: none;
top: 5px;
width: auto;
z-index: 100000;
}
`
// Todo: Refactor to from isHidden boolean
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

&& we might not even need a component for this at all if all we're doing is importing Chakra's primitive 🤔?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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>
)}

Copy link
Member

@pettinarip pettinarip Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I will get to this when I have some free evening time this week.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can just import this context though instead of this. Still ways to clean it up.


export interface IProps {
children?: React.ReactNode
isHidden?: boolean
}

const VisuallyHidden: React.FC<IProps> = ({ isHidden = false, children }) =>
isHidden ? <Container>{children}</Container> : <>{children}</>
isHidden ? (
<VisuallyHiddenSpan>{children}</VisuallyHiddenSpan>
) : (
<>{children}</>
)

export default VisuallyHidden