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

Allow use of externally defined styles #143

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joeflack4
Copy link
Contributor

Allowed internal component styles to be ignored, in the case that external styles are being used instead.

@joeflack4 joeflack4 force-pushed the pr_external-styles-only branch from 986be5f to 2039500 Compare February 10, 2018 03:06
const overlayStyle = {...defaultStyles.overlay, ...this.props.styles.overlay};
const sidebarStyle = {...this.props.externalStylesOnly ? {} : defaultStyles.sidebar, ...this.props.styles.sidebar};
const contentStyle = {...this.props.externalStylesOnly ? {} : defaultStyles.content, ...this.props.styles.content};
const overlayStyle = {...this.props.externalStylesOnly ? {} : defaultStyles.overlay, ...this.props.styles.overlay};

Choose a reason for hiding this comment

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

'spread/rest operator' is only available in ES6 (use 'esversion: 6').
Expected an identifier and instead saw 'this' (a reserved word).
Expected ')' to match '}' from line 222 and instead saw '.'.
'function closure expressions' is only available in Mozilla JavaScript extensions (use moz option).
Unnecessary semicolon.

const contentStyle = {...defaultStyles.content, ...this.props.styles.content};
const overlayStyle = {...defaultStyles.overlay, ...this.props.styles.overlay};
const sidebarStyle = {...this.props.externalStylesOnly ? {} : defaultStyles.sidebar, ...this.props.styles.sidebar};
const contentStyle = {...this.props.externalStylesOnly ? {} : defaultStyles.content, ...this.props.styles.content};

Choose a reason for hiding this comment

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

Class properties must be methods. Expected '(' but instead saw 'contentStyle'.
Expected '(' and instead saw '}'.
Expected an identifier and instead saw ':'.
'spread/rest operator' is only available in ES6 (use 'esversion: 6').
Expected an identifier and instead saw 'this' (a reserved word).
Expected ')' to match '}' from line 222 and instead saw '.'.
'function closure expressions' is only available in Mozilla JavaScript extensions (use moz option).
Expected an identifier and instead saw ';'.

const sidebarStyle = {...defaultStyles.sidebar, ...this.props.styles.sidebar};
const contentStyle = {...defaultStyles.content, ...this.props.styles.content};
const overlayStyle = {...defaultStyles.overlay, ...this.props.styles.overlay};
const sidebarStyle = {...this.props.externalStylesOnly ? {} : defaultStyles.sidebar, ...this.props.styles.sidebar};

Choose a reason for hiding this comment

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

'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
Expected '}' to match '{' from line 221 and instead saw '...'.
Missing semicolon.
'spread/rest operator' is only available in ES6 (use 'esversion: 6').
Expected an assignment or function call and instead saw an expression.
Unnecessary semicolon.

@markusenglund
Copy link
Collaborator

Seems reasonable, I can definitely see how this would be useful to many developers.

I just have one objection with the following code:

const sidebarStyle = {...this.props.externalStylesOnly ? {} : defaultStyles.sidebar, ...this.props.styles.sidebar};

The styles that the user has explicitly passed in through the styles prop are ignored if they have the externalStylesOnly flag enabled. I think they should still be included, otherwise users who just wanted to disable the default styles will be confused.

Maybe the above code snippet should look something like this instead:

const sidebarStyle = {...this.props.externalStylesOnly ? this.props.styles.sidebar : defaultStyles.sidebar, ...this.props.styles.sidebar};

What do you think?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants