Used styled components in Sidebar

[adityagarg06]

Hey @lindapaiste, I've been working on adding styled component to the Sidebar. I've come across a situation where the class .sidebar_project-options needs to extend from .dropdown-open-right. However, since I can't directly use extends in styled components, I'm considering moving the dropdown-open-right code to the mixins.scss file as mixins. Would it be a good idea, or do you have any other suggestions?

It's coming up like this without the dropdown-open-right

.sidebar__project-options {
  @extend %dropdown-open-right;
  display: none;
  width: 100%;
  max-width: #{180 / $base-font-size}rem;
  .sidebar--project-options & {
    display: flex;
  }
}

I have verified that this pull request:

• has no linting errors (npm run lint)
• has no test errors (npm run test)
• is from a uniquely-named feature branch and is up to date with the develop branch.
• is descriptively named and links to an issue number, i.e. Fixes #123

[lindapaiste]

Hey @lindapaiste, I've been working on adding styled component to the Sidebar. I've come across a situation where the class .sidebar_project-options needs to extend from .dropdown-open-right. However, since I can't directly use extends in styled components, I'm considering moving the dropdown-open-right code to the mixins.scss file as mixins. Would it be a good idea, or do you have any other suggestions?

This has been something that has been on my mind and I'm not sure of the best long-term solution. The best temporary solution to keep us moving for now is to keep a className on the sidebar div in order to apply the CSS mixin. In the SCSS, you would delete all of the styles that you migrated to styled-components, but keep the @extend mixin.

.sidebar__project-options {
  @extend %dropdown-open-right;
}

<SidebarProjectOptions className="sidebar__project-options">

[lindapaiste]

I have some thoughts about conditional rendering and I don't know if others agree @raclim @dewanshDT. This is just my opinion but I don't like hiding content via CSS display: none. This code (and other components with similar setups) could definitely be simplified a lot if we were to handle the conditional rendering in the JS instead. And I think it's better for accessibility because we don't need to worry about adding aria-hidden attributes if the hidden elements aren't in the DOM.

@adityagarg06 The way you have handled the showing and hiding is consistent with how it's being done currently in the .scss so that's good ✔️. It's probably something that we should discuss and get everyone on board with before making the changes that I am suggesting.

---

Instead of this:

const SidebarProjectOptions = styled.ul`
  display: none;
  width: 100%;
  max-width: ${remSize(180)};
  ${(props) =>
    props.open &&
    css`
      display: flex;
    `}
`;

<SidebarProjectOptions open={projectOptionsVisible}>
  /*...*/
</SidebarProjectOptions>

It could be:

const SidebarProjectOptions = styled.ul`
  display: flex;
  width: 100%;
  max-width: ${remSize(180)};
`;

{projectOptionsVisible && (
  <SidebarProjectOptions>
    /*...*/
  </SidebarProjectOptions>
)}

[dewanshDT]

I totally agree with this

[adityagarg06]

@lindapaiste I have made all the changes that you required, Sorry for the late response.

[vaishnavi192]

This issue is closed right? @adityagarg06 @lindapaiste

[raclim]

Thanks for your work on this! Due to the amount of time that's passed since this pull request was opened, I'm going to close this for now. I'm sorry that we ended up not being able to merge this in, but please feel free to reopen a new pull request for this issue!