Skip to content

Migrate SCSS to styled-components #1760

New issue
New issue
Open
Open
Migrate SCSS to styled-components#1760
Labels
Priority:HighShould be addressed soon but not criticalType:TaskTasks tied specifically to developer operations and maintenance
@catarak

Description

@catarak
catarak
opened on Feb 11, 2021
Member

Nature of issue?

  • Existing feature enhancement

Feature enhancement details:

styled-components has already been added to the web editor. It will be a long process to remove all of the SCSS but it will be worth it ✨

Activity

catarak
added
Priority:HighShould be addressed soon but not critical
Type:TaskTasks tied specifically to developer operations and maintenance
on Feb 11, 2021
catarak
mentioned this on Feb 11, 2021
rt1301

rt1301 commented on Feb 12, 2021

@rt1301
rt1301
on Feb 12, 2021
Contributor

@catarak I would like to work on this issue if it is open. Can you suggest me how to get started on this?

Ajaya1000

Ajaya1000 commented on Feb 17, 2021

@Ajaya1000
Ajaya1000
on Feb 17, 2021 · edited by Ajaya1000
Contributor

@catarak as this is a pretty long implementation I would like to work with @rt1301 to speed up the process. Can you please guide us.

catarak

catarak commented on Feb 19, 2021

@catarak
catarak
on Feb 19, 2021
MemberAuthor

This is a really big task and I would suggest starting small. Try to convert one component, one small component. Transform a SCSS placeholder into a generic component. Something like that.

catarak
mentioned this on Mar 3, 2021
satyasaibhushan
mentioned this on Mar 5, 2021
satyasaibhushan

satyasaibhushan commented on Mar 5, 2021

@satyasaibhushan
satyasaibhushan
on Mar 5, 2021
Contributor

Hey @catarak,
I've converted a small component, please check and give your suggestions and thoughts!

satyasaibhushan
mentioned this on Mar 5, 2021
smitbarmase
mentioned this on Mar 14, 2021
Ajaya1000
added 2 commits that reference this issue on Mar 15, 2021

[processing#1760]Added logoColor

4cfe760

[processing#1760] About Component scss to styled component

445115d
Ajaya1000

Ajaya1000 commented on Mar 15, 2021

@Ajaya1000
Ajaya1000
on Mar 15, 2021
Contributor

Hi @catarak . I have worked on the "About Component". While working on the issue I faced a challenge where themify mixin was used. Please suggest to me how to counter the problem.

Ajaya1000
added a commit that references this issue on Mar 15, 2021

[processing#1760] fixed issue with hovering

acded83

9 remaining items

rt1301

rt1301 commented on Mar 17, 2021

@rt1301
rt1301
on Mar 17, 2021 · edited by rt1301
Contributor

@catarak I am trying to convert the NavBasic.jsx file to styled component. I am having difficulties converting the following "theme mixin" code:
@include themify() { border-bottom: 1px dashed map-get($theme-map, 'nav-border-color'); }
Can you suggest to me how to solve this?

satyasaibhushan

satyasaibhushan commented on Mar 17, 2021

@satyasaibhushan
satyasaibhushan
on Mar 17, 2021 · edited by satyasaibhushan
Contributor

Hey @catarak
I don't like the way this issue is going forward.Please assign this to someone and let it go in one single direction with a clear idea on how to tackle this.
Please do it fast as many are trying to contribute to this which may lead to some work done which is not used in the repo.

catarak

catarak commented on May 4, 2021

@catarak
catarak
on May 4, 2021
MemberAuthor

Sorry for the delay in responding to this issue and all of the related pull requests! I was on a GitHub hiatus and now I am making my way through issues/PRs. I agree with @satyasaibhushan, that this needs to be approached component by component with small PRs rather than with large PRs. It will be too difficult for me to review huge PRs with a lot of changes, and unfortunately I'm going to have to close those PRs.

lindapaiste

lindapaiste commented on May 13, 2023

@lindapaiste
lindapaiste
on May 13, 2023 · edited by lindapaiste
Collaborator

I have questions about the preferred code style for implementing styled-components @raclim @dewanshDT

This is very subjective but I'll lay out some pros and cons with examples so you can see what I mean.

Question 1: Should each component use one styled element or many?

Approach A: Use styled separately on each DOM element.

Advantages:

  • Makes it easier to export and reuse individual elements (ie. PageTitle).
  • Encapsulation - the styles of PageTitle don't depend on its context.
  • Easier to see what styles are applied to a specific element.
import styled from "styled-components";

const PageContainer = styled.div`
  padding: 24px;
`;

const PageTitle = styled.h1`
  font-size: 28px;
`;

const PageParagraph = styled.p`
  font-size: 14px;
`;

export default function Page() {
  return (
    <PageContainer>
      <PageTitle>Hello World</PageTitle>
      <PageParagraph>Lorem impsum body text.</PageParagraph>
      <PageParagraph>More lorem impsum text.</PageParagraph>
    </PageContainer>
  );
}

Approach B: Use nested CSS to style child elements of on main styled component

Note: this can be done by targeting elements like h1, p, etc. or class names like .title.

Advantages:

  • Fewer lines of code.
  • Easier to convert existing components which use CSS files.
  • Can apply conditional styles to children based on classes (see section 2B).
import styled from "styled-components";

const PageStyle = styled.div`
  padding: 24px;
  h1 {
    font-size: 28px;
  }
  p {
    font-size: 14px;
  }
`;

export default function Page() {
  return (
    <PageStyle>
      <h1>Hello World</h1>
      <p>Lorem impsum body text.</p>
      <p>More lorem impsum text.</p>
    </PageStyle>
  );
}

Question 2: Should conditional styling be applied through props or classes?

Approach A: Styles depend on the props of the styled component

Advantages:

  • This is the most "pure" implementation of styled-components because it doesn't need any CSS class names.
  • Encapsulation.
import styled from "styled-components";

const List = styled.ul`
 list-style-type: none;
 margin: 20px;
 padding: 0;
 width: 180px;
`;

const ListItem = styled.li`
 font-size: 14px;
 padding: 4px 8px;
 background: ${(props) => (props.isSelected ? "lightgray" : "transparent")};
`;

export default function Menu() {
 return (
   <List>
     <ListItem>Deutsch</ListItem>
     <ListItem isSelected>English</ListItem>
     <ListItem>Español</ListItem>
   </List>
 );
}

Approach B: Styles depend on class names

Advantages:

  • Easier to convert existing components which use classNames to combine multiple conditional classes.
  • Can use classes on the top-level styled element or on deeply nested components. In other words, this works with both 1A and 1B whereas 2A only works with 1A.
  • Don't need to worry about declaring extra props in propTypes.
  • Works better alongside the existing prop utility function for accessing theme variables. This utility returns a function of props so it needs special handling to work within a ${(props) => function (ie. ${(props) => props.isSelected ? prop('primaryTextColor')(props) : prop('secondaryTextColor')(props)})
import styled from "styled-components";

const List = styled.ul`
  list-style-type: none;
  margin: 20px;
  padding: 0;
  width: 180px;
  li {
    font-size: 14px;
    padding: 4px 8px;
    background: transparent;
  }
  li.selected {
    background: lightgray;
  }
`;

export default function Menu() {
  return (
    <List>
      <li>Deutsch</li>
      <li className="selected">English</li>
      <li>Español</li>
    </List>
  );
}

or

import styled from "styled-components";

const List = styled.ul`
  list-style-type: none;
  margin: 20px;
  padding: 0;
  width: 180px;
`;

const ListItem = styled.li`
  font-size: 14px;
  padding: 4px 8px;
  background: transparent;
  &.selected {
    background: lightgray;
  }
`;

export default function Menu() {
  return (
    <List>
      <ListItem>Deutsch</ListItem>
      <ListItem className="selected">English</ListItem>
      <ListItem>Español</ListItem>
    </List>
  );
}

Example codes on CodeSandbox

dewanshDT

dewanshDT commented on May 13, 2023

@dewanshDT
dewanshDT
on May 13, 2023
Collaborator

hey @lindapaiste I think we should go with 1A and 2B, I agree with the advantages that come with them.

raclim

raclim commented on May 17, 2023

@raclim
raclim
on May 17, 2023
Collaborator

Thanks @lindapaiste for the thorough explanations and examples! I'm not too familiar with styled-components, so my understanding of this might need some clarification!

I'm leaning towards 1B and 2B particularly because of their ease in converting existing components (and I think it's also how it's currently implemented in the mobile components?). However, I also like the readability and cleanliness of 1A, and might be better to implement for the longterm because of the listed advantages. Ultimately though, I think you and @dewanshDT should go with what feels best and makes sense for the both of you! Is there an approach you preferred as well?

adityagarg06

adityagarg06 commented on Jul 31, 2023

@adityagarg06
adityagarg06
on Jul 31, 2023 · edited by adityagarg06
Contributor

@lindapaiste I was adding the styled-component to the About.jsx file. I added the logoColor property in the theme.js but its giving error that prop logoColor not found. I have added logoColor like this under light, dark and contract

const baseThemes = {
  [Theme.light]: {
    colors,
    ...common,
    primaryTextColor: grays.dark,
    inactiveTextColor: grays.middleDark,
    backgroundColor: grays.lighter,
    logoColor: colors.p5jsPink,

About.jsx

import React from 'react';
import { useSelector } from 'react-redux';
import { Helmet } from 'react-helmet';
import { useTranslation } from 'react-i18next';
import { Link } from 'react-router';
import SquareLogoIcon from '../../../images/p5js-square-logo.svg';
// import PlayIcon from '../../../images/play.svg';
import AsteriskIcon from '../../../images/p5-asterisk.svg';
import packageData from '../../../../package.json';
import styled, { css } from 'styled-components';
import { remSize, prop } from '../../../theme';

const AboutContent = styled.div`
  display: flex;
  flex-direction: row;
  justify-content: space-between;
  flex-wrap: wrap;
  padding-top: ${remSize(17)};
  padding-right: ${remSize(78)};
  padding-bottom: ${remSize(20)};
  padding-left: ${remSize(20)};
  width: ${remSize(720)};

  @media (max-width: 768px) {
    flex-direction: column;
    overflow-y: auto;
    overflow-x: hidden;
    width: 100%;
    margin-right: ${remSize(15)};
  }
`;

const AboutContentColumn = styled.div`
  display: flex;
  flex-direction: column;
  margin-left: 15px;
  margin-right: 15px;
`;

const AboutContentColumnTitle = styled.h3`
  font-size: ${remSize(21)};
  padding-left: ${remSize(17)};
`;

const AboutContentColumnAsterisk = styled.div`
  padding-right: ${remSize(5)};
  ${props => props.theme && css`
    & path {
      fill: ${prop('logoColor')(props.theme)};
      stroke: ${prop('logoColor')(props.theme)};
    }
  `}
`;

const AboutContentColumnList = styled.p`
  ${props => props.theme && css`
    padding-top: ${remSize(10)};
    font-size: ${remSize(16)};
  `}
`;

const AboutVersionInfo = styled.p`
  ${props => props.theme && css`
    padding-top: ${remSize(8)};
    font-size: ${remSize(16)};
    span {
      color: ${prop('logoColor')(props.theme)};
    }
    &:first-child {
      padding-top: ${remSize(30)};
    }
  `}
`;

const AboutFooter = styled.div`
  display: flex;
  justify-content: space-between;
  padding-top: ${remSize(18)};
  padding-right: ${remSize(20)};
  padding-bottom: ${remSize(21)};
  padding-left: ${remSize(20)};
  width: 100%;

  @media (max-width: 768px) {
    flex-direction: column;
    padding-left: ${remSize(20)};
    padding-right: ${remSize(20)};
  }
`;

const AboutFooterList = styled.p`
  padding-top: ${remSize(12)};
`;

const AboutLogo = styled.div`
  ${props => props.theme && css`
    & path {
      fill: ${prop('logoColor')(props.theme)};
    }
  `}
`;
lindapaiste

lindapaiste commented on Jul 31, 2023

@lindapaiste
lindapaiste
on Jul 31, 2023
Collaborator

@adityagarg06 The prop utility is a higher-order function so it actually returns a function of props. In your styled components you write color: ${prop('logoColor')}; and that's the equivalent of color: ${(props) => props.theme.logoColor};. There are no other styles in those components that depend on props so you don't need to have any ${props => or css` .

It should look like this:

const AboutVersionInfo = styled.p`
  padding-top: ${remSize(8)};
  font-size: ${remSize(16)};
  span {
    color: ${prop('logoColor')};
  }
  &:first-child {
    padding-top: ${remSize(30)};
  }
`;

If there's a className on a React component that's not a built-in DOM element (looks like AsteriskIcon is the only one on this page) you can still use styled on it.

const AboutContentColumnAsterisk = styled(AsteriskIcon)`
  padding-right: ${remSize(5)};
  & path {
    fill: ${prop('logoColor')};
    stroke: ${prop('logoColor')};
  }
`;

(BTW, I'm looking at this About component right now and it really irks me that we have a "list" which is not semantically a list because we're not using ul and li. But one thing at a time...)

adityagarg06

adityagarg06 commented on Aug 1, 2023

@adityagarg06
adityagarg06
on Aug 1, 2023
Contributor

@lindapaiste Thanks for helping, I have fixed the props but some styles are getting overridden by _mixin.scss.

lindapaiste

lindapaiste commented on Aug 1, 2023

@lindapaiste
lindapaiste
on Aug 1, 2023
Collaborator

@lindapaiste Thanks for helping, I have fixed the props but some styles are getting overridden by _mixin.scss.

Ugh yeah I have experienced that and it's really annoying. Feel free to put up a draft PR with what you have now.

One of the tricks that we used is to wrap the style in &&& { } to increase the specificity, like we do here.

(For the record, I don't like that we use the &&& in core UI elements like Button and icons because it makes it hard to override when we want to extend it. But that's not an issue with the About components as they're not used anywhere else.)

adityagarg06
mentioned this on Aug 2, 2023
lindapaiste
mentioned this on Aug 6, 2023
lindapaiste
mentioned this on Sep 20, 2023
adityagarg06
mentioned this on Dec 28, 2023
adityagarg06
mentioned this on Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Priority:HighShould be addressed soon but not criticalType:TaskTasks tied specifically to developer operations and maintenance

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @catarak@lindapaiste@satyasaibhushan@Ajaya1000@raclim

      Issue actions
        Migrate SCSS to styled-components · Issue #1760 · processing/p5.js-web-editor