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.

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

added styled component to About.jsx #2342

Closed

Conversation

adityagarg06
Copy link
Contributor

Partially Fixes #1760

Changes:
Added styled component to the About.jsx file. However, the styles get overridden by the _mixin.scss.

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

@adityagarg06
Copy link
Contributor Author

@lindapaiste I have wrapped the style around &&& {} but still it gets overridden by _mixin.scss

@lindapaiste
Copy link
Collaborator

Logo

When you use styled(Component) on an existing component you are creating an alternate version of that component which you use in the same way as the original. It will take accept all of the same props and pass them through. Instead of <SquareLogoIcon as={AboutLogo} ... what you want is <AboutLogo ....

Asterisks

Replace each

<AsteriskIcon
  as={AboutContentColumnAsterisk}
  aria-hidden="true"
  focusable="false"
/>

With

<AboutContentColumnAsterisk />

It would be annoying to have to put <AboutContentColumnAsterisk aria-hidden="true" focusable="false" /> each time. We can add the aria-hidden="true" focusable="false" to all of them like this:

const AboutContentColumnAsterisk = styled(AsteriskIcon).attrs({
  ariaHidden: true,
  focusable: false
})`
  &&& {
    padding-right: ${remSize(5)};
    & path {
      fill: ${prop('logoColor')};
      stroke: ${prop('logoColor')};
    }
  }
`;

Footer

The main reason that this isn't looking right is that you've got it inside of an <AboutContentColumn/> when it was previously a sibling.

image
The way it's laid out now is that the two elements are spaced apart with justify-content: space-between; and then they are shifted over to the right side with a large margin on the left. That current structure is a bit weird IMO so feel free to write it differently if you have something better in mind.

When I'm comparing localhost to production I can see that there's a bit of a gap on the left due to the margin-left: 15px; from your previous PR. Was that intentional?

Also the breakpoints are not quite right but it's showing potential.
image
You've got it switching from row to column but it needs a flex-wrap: nowrap; when on the smaller size.

@lindapaiste lindapaiste added this to Reviewing in Progress in PR Review Board Aug 2, 2023
@lindapaiste lindapaiste added this to Styled-Components in Code Cleanup Aug 2, 2023
@adityagarg06
Copy link
Contributor Author

@lindapaiste I have made all the changes you suggested, and everything works accordingly. Thank you so much for your help.

@lindapaiste
Copy link
Collaborator

Thanks for fixing everything! The only difference that I still see is this:

When I'm comparing localhost to production I can see that there's a bit of a gap on the left due to the margin-left: 15px; from your previous PR. Was that intentional?

Production
image
Localhost
image

Also the hover effect on the asterisk icons is different, but the previous effect looks like it was an accident (it changes the fill but not the stroke) so yours is probably an improvement. In the future we may want a more obvious hover effect on the links, like maybe a pink underline on hover.

@raclim
Copy link
Collaborator

raclim commented Jan 26, 2024

I think ideally the About modal will undergo some larger design changes within the near future, so I'm going to close this for now.

@raclim raclim closed this Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Code Cleanup
Styled-Components
PR Review Board
Reviewing in Progress
Development

Successfully merging this pull request may close these issues.

Migrate SCSS to styled-components
3 participants