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

style(Loader|Reveal|Segment): propTypes cleanups and typings update #1165

Merged
merged 4 commits into from
Jan 17, 2017

Conversation

layershifter
Copy link
Member

This PR is part of work for removing propTypes in production bundle (#524, #731).
Also, cleanups and updates typings for #1072.

Affects following components:

  • Loader
  • Reveal
  • Segment

@codecov-io
Copy link

codecov-io commented Jan 15, 2017

Current coverage is 95.88% (diff: 100%)

No coverage report found for master at b7ffa6a.

Powered by Codecov. Last update b7ffa6a...3b24602

Copy link
Member

@levithomason levithomason left a comment

Choose a reason for hiding this comment

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

Just one note about the common test.

const { assertRequired } = commonTestHelpers('implementsTextAlignProp', Component)

describe('aligned (common)', () => {
assertRequired(Component, 'a `Component`')

_noDefaultClassNameFromProp(Component, 'textAlign', options)
_noClassNameFromBoolProps(Component, 'textAlign', options)

_.each(Component._meta.props.aligned, (propVal) => {
Copy link
Member

Choose a reason for hiding this comment

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

The removed portion of the test is the useful part. It makes the assertion that the proper className is added. I see it was removed as it looped over _meta.props.aligned. In fact, this test was doing nothing as the aligned prop name was changed to textAlign!

Instead of removing these assertions, however, let's loop over SUI.TEXT_ALIGNMENTS directly:

_.each(SUI.TEXT_ALIGNMENTS, (propVal) => {
  ...
})

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm think we can do there something better 🤔 We can use this pattern with other common tests that relies on meta. Thoughts?

export const implementsTextAlignProp = (Component, alignments = SUI.TEXT_ALIGNMENTS,  options = {}) => {
// ...
  _.each(alignments, (propVal) => {
  // ...
  })
}

Copy link
Member

Choose a reason for hiding this comment

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

That would also work. I checked all the usages and all components that implement TEXT_ALIGNMENTS implement all of them. I'm OK with the default pattern as well.

@@ -854,17 +859,36 @@ export const implementsImageProp = (Component, options = {}) => {
/**
* Assert that a Component correctly implements the "textAlign" prop.
* @param {React.Component|Function} Component The component to test.
* @param {array} alignments Array of possible alignment positions.
Copy link
Member

Choose a reason for hiding this comment

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

This can be optional:

-@param {array} alignments
+@param {array} [alignments]

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed 😄

@levithomason
Copy link
Member

Awesome, thank you!

@levithomason levithomason merged commit cae54eb into master Jan 17, 2017
@levithomason levithomason deleted the style/proptypes-cleanups-3 branch January 17, 2017 19:00
@levithomason
Copy link
Member

Released in [email protected].

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

Successfully merging this pull request may close these issues.

3 participants