Skip to content
This repository has been archived by the owner on May 19, 2020. It is now read-only.

Reduce Prop Reliance #255

Closed
irmerk opened this issue Mar 2, 2020 · 5 comments
Closed

Reduce Prop Reliance #255

irmerk opened this issue Mar 2, 2020 · 5 comments
Assignees

Comments

@irmerk
Copy link
Member

irmerk commented Mar 2, 2020

We should not be in the habit of relying on a lot of styling props for individual fields. This practice has gotten out of hand:

editorProps = {
    BUTTON_BACKGROUND_INACTIVE (string),
    BUTTON_BACKGROUND_ACTIVE (string),
    BUTTON_SYMBOL_INACTIVE (string),
    BUTTON_SYMBOL_ACTIVE (string),
    DROPDOWN_COLOR (string),
    EDITOR_BORDER (string),
    EDITOR_BORDER_RADIUS (string),
    EDITOR_HEIGHT (string),
    EDITOR_MARGIN (string),
    EDITOR_SHADOW (string),
    EDITOR_WIDTH (string),
    TOOLBAR_BACKGROUND (string),
    TOOLTIP_BACKGROUND (string),
    TOOLTIP (string),
    TOOLBAR_SHADOW (string),
};

All of these should be removed and we should instead rely on a strong system of structured class names. This will allow users to handle what these props currently do via overriding the CSS.

Related Issue:

@irmerk irmerk added Good First Issue :octocat: Good for newcomers Difficulty: Medium Type: Enhancement ✨ Improvement to process or efficiency labels Mar 2, 2020
@irmerk irmerk added this to Triage in Cicero UI v1.0 via automation Mar 2, 2020
@98lenvi
Copy link
Contributor

98lenvi commented Mar 2, 2020

Please assign this to me!

@irmerk irmerk added Good First Issue (Taken) and removed Good First Issue :octocat: Good for newcomers labels Mar 2, 2020
@98lenvi
Copy link
Contributor

98lenvi commented Mar 3, 2020

@irmerk I have a question, wouldn't removing props all together and introducing class-name based styling be opposite to what styled-components actually is?

I think defining the editorProps like this and passing this down to the components, and at the same time declaring the same object as default props will make the code look much cleaner and at the same time use CSS in JS. I can also modify the styled-components in such a way that it accepts a wide option of user styling. (Can be achieved by making every component extend a base component)

`

button : {
  inActive : {
    symbol : PropTypes.string,
    background : PropTypes.string,
  },
  active : {
    symbol : PropTypes.string,
    background : PropTypes.string,
  },
  dropDown : {
    color : PropTypes.string
  },
  editor : {
    border : {
    },
    height : PropTypes.string,
    width : PropTypes.string,
    margin : PropTypes.string,
    shadow : PropTypes.string,
  },
  toolbar : {
    background : PropTypes.string,
  },
  toolTip : {
    background : PropTypes.string,
    tooltip : PropTypes.string,
    shadow : PropTypes.string,
  }
,`

and similarly an object like above as default props, this would remove the necessity of the functions in toolbarStyles.js

This doesn't mean I'm against using CSS for overriding, I wanted to know if this approach would be better?

@irmerk
Copy link
Member Author

irmerk commented Mar 4, 2020

My initial thought was to remove the conditional props and let styled components do its thing. The user would then provide an override to the styled components via the class system if they want different styling.

@mttrbrts @DianaLease thoughts?

@98lenvi
Copy link
Contributor

98lenvi commented Mar 4, 2020

@irmerk. Okay, I got it! So I'll start off writing the classnames.

@irmerk irmerk moved this from Triage to Priority: High in Cicero UI v1.0 Apr 30, 2020
@irmerk
Copy link
Member Author

irmerk commented May 4, 2020

I believe this is closed with the Slate migration.

@irmerk irmerk closed this as completed May 4, 2020
Cicero UI v1.0 automation moved this from Priority: High to Finished Issues May 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Cicero UI v1.0
  
Finished Issues
Development

No branches or pull requests

2 participants