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

Revisit constants and configurability #440

Closed
mtscout6 opened this issue Mar 5, 2015 · 2 comments
Closed

Revisit constants and configurability #440

mtscout6 opened this issue Mar 5, 2015 · 2 comments
Labels

Comments

@mtscout6
Copy link
Member

mtscout6 commented Mar 5, 2015

Issue #306 highlights the ability to modify the constants used for prop validation and mapping. The name constants is deceiving since they are not truly constant values. We should give some thought to this practice give it a proper name. When we do this we should add some documentation for those wishing to work with custom classes. Issue #404 is a fine example of such a case.

@mtscout6 mtscout6 added enhancement help wanted docs Documentation related labels Mar 5, 2015
@mtscout6 mtscout6 changed the title Rename constants Revisit constants and configurability Apr 6, 2015
@trevorr
Copy link

trevorr commented Apr 8, 2015

Perhaps the right fix is to add isKeyOf to CustomPropTypes and use that within the propTypes definition of BootstrapMixin, so that additions to STYLES are used? I think it would also be better to add a function to BootstrapMixin to define new styles, rather than modifying constants. That would provide more control (e.g. add but not remove), clarity (STYLES being a map is strange from an external perspective), and debuggability (a place to set a breakpoint when styles are added), and it's slightly less of an exposed implementation detail. It might be reasonable to then make constants private again. Thoughts?

@dozoisch dozoisch added this to the 1.0.0 Release milestone May 3, 2015
@AlexKVal AlexKVal removed this from the 1.0.0 Release milestone Jun 29, 2015
@AlexKVal
Copy link
Member

Superceded by #646

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

No branches or pull requests

4 participants