Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

refactor: Add "style" to StandardProps and implement in all components #219

Merged
merged 31 commits into from
Aug 13, 2019

Conversation

alexpaxton
Copy link
Contributor

@alexpaxton alexpaxton commented Aug 13, 2019

Closes #217
Closes #213

Changes

  • Add style to StandardProps
  • Ensure style prop was being passed into the outermost element (in most cases)
  • Ensure className prop was being passed through correctly in TextArea
  • Update docs for TextBlock and Icon components to expose style prop

Checklist

Check all that apply

  • Updated documentation to reflect changes
  • Added entry to top of Changelog with link to PR (not issue)
  • Tests pass
  • Peer reviewed and approved
  • Signed CLA (if not already signed)

@alexpaxton alexpaxton requested review from mavarius and a team August 13, 2019 18:20
@ghost ghost requested review from 121watts and removed request for a team August 13, 2019 18:20
@alexpaxton alexpaxton merged commit 5f598cb into master Aug 13, 2019
@alexpaxton alexpaxton deleted the refactor/make-style-standard branch August 13, 2019 20:54
Copy link
Collaborator

@mavarius mavarius left a comment

Choose a reason for hiding this comment

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

A few comments, mostly the same issue really.

}

return {width: '100%'}
return {...style, width: '100%'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default should be reversed, {width: '100%', ...style}, so width can be overridden if provided.


interface Props extends StandardProps {
/** Icon to display */
glyph: IconFont | string
/** Optional color string, can use InfluxColors enum or pass in your own value */
color?: InfluxColors | string
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be a breaking change, we'll need to keep a tab on this when updating the libraries. Maybe also call it out specifically in the patch notes.

}

return {width: '100%'}
return {...style, width: '100%'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default should be reversed, {width: '100%', ...style}, so width can be overridden if provided.

}

return {width: '100%'}
return {...style, width: '100%'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default should be reversed, {width: '100%', ...style}, so width can be overridden if provided.


return {maxWidth: `${maxWidth}px`}
return {...style, maxWidth: `${maxWidth}px`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

...style should be the last item in the default return.


const borderWidth = `${this.strokeWidth}px`
const width = `${diameterPixels}px`
const height = `${diameterPixels}px`

return {width, height, borderWidth}
return {...style, width, height, borderWidth}
Copy link
Collaborator

Choose a reason for hiding this comment

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

...style should be the last item in the default.


return {width}
return {...style, width}
Copy link
Collaborator

Choose a reason for hiding this comment

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

...style should be the last item in the default.


return {
...style,
Copy link
Collaborator

Choose a reason for hiding this comment

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

...style should be the last item in the default.


return {
...style,
Copy link
Collaborator

Choose a reason for hiding this comment

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

...style should be the last item in the default.

@@ -74,7 +74,7 @@ export class TextBlock extends Component<Props> {
color = `${textColor}`
}

return {backgroundColor, color}
return {...style, backgroundColor, color}
Copy link
Collaborator

Choose a reason for hiding this comment

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

...style should be the last item in the default.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "style" to StandardProps TextArea does not support className prop
3 participants