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

[added] styleMaps.addStyle and CustomPropTypes.keyOf #496

Merged
merged 1 commit into from
Apr 20, 2015

Conversation

trevorr
Copy link

@trevorr trevorr commented Apr 8, 2015

Proposed fix for #306 and partial fix for #440.

let valuesString = JSON.stringify(Object.keys(obj));
return new Error(
('Invalid prop `' + propName + '` of value `' + propValue + '` ') +
('supplied to `' + componentName + '`, expected one of ' + valuesString + '.')
Copy link
Member

Choose a reason for hiding this comment

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

We're using ES6 you can use string interpolation here to clean this up: http://babeljs.io/docs/learn-es6/#template-strings

Copy link
Author

Choose a reason for hiding this comment

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

Would that be preferable even with needing to escape the backticks within the string? I basically copied the code from ReactPropTypes createOneOfChecker. The surrounding code also doesn't use interpolation.

Copy link
Member

Choose a reason for hiding this comment

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

Is there any major advantage to backticks in the error message? We could just change them to quotes.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't change everything to ES6 templates when I switched everything to ES6, I'm just trying to keep things as clean as possible going forward.

Copy link
Author

Choose a reason for hiding this comment

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

That's fine with me. I think they're a bit odd anyway. The only issue is consistency with React error messages, but that seems minor.

@mtscout6
Copy link
Member

This looks really great I like the simplicity of it. We should really add some documentation for this though so folks are aware that this is possible without combing the issues to find it. The Components page does not feel right for this though. I'm wondering if we should add a "Customize" page to the docs. This is partly why I feel that the addStyles function feels wrong on the BootstrapMixin mixins are generally tied directly to components and this configuration would be modifiable outside of components.

@trevorr trevorr changed the title [added] BootstrapMixin.addStyle and CustomPropTypes.keyOf [added] styleMaps.addStyle and CustomPropTypes.keyOf Apr 10, 2015
@trevorr
Copy link
Author

trevorr commented Apr 13, 2015

Perhaps splitting off the styles from the other constants would be better, since they're the only ones likely to be extended by users? They could be in a module called Styles and the rest could remain (unexported) in constants.

@mtscout6
Copy link
Member

#404 Is one such example where the Classes could be modified. But you're right that the Sizes and Glyphs should need to be exported.

@mtscout6
Copy link
Member

The code looks great! Can you squash the commits with a commit message following the Contributing Guidelines? A commit message of that formate will reflect the change in the automated changelog when we release.

@trevorr
Copy link
Author

trevorr commented Apr 17, 2015

Squashed and also fixed a warning by removing unused import React from BootstrapMixin.

mtscout6 added a commit that referenced this pull request Apr 20, 2015
[added] styleMaps.addStyle and CustomPropTypes.keyOf
@mtscout6 mtscout6 merged commit 579d170 into react-bootstrap:master Apr 20, 2015
@mtscout6
Copy link
Member

Thanks!

@mtscout6 mtscout6 mentioned this pull request Apr 20, 2015
@trevorr trevorr deleted the issue-440 branch April 5, 2016 16:09
aryad14 pushed a commit to aryad14/react-bootstrap that referenced this pull request Oct 11, 2023
…p#496)

* chore(package): update enzyme to version 3.9.0

* chore(package): update enzyme-adapter-react-16 to version 1.9.1

* chore(package): update lockfile package-lock.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants