feat(priceRanges): Add BEM classes and tests #390
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| var React = require('react'); | ||
|
|
||
| var Template = require('../Template'); | ||
| var PriceRangesForm = require('./PriceRangesForm'); | ||
| var cx = require('classnames'); | ||
|
|
||
| class PriceRanges extends React.Component { | ||
| getForm() { | ||
| return ( | ||
| <PriceRangesForm | ||
| cssClasses={this.props.cssClasses} | ||
| labels={this.props.labels} | ||
| refine={this.refine.bind(this)} | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| getURLFromFacetValue(facetValue) { | ||
| if (!this.props.createURL) { | ||
| return '#'; | ||
| } | ||
| return this.props.createURL(facetValue.from, facetValue.to, facetValue.isRefined); | ||
| } | ||
|
|
||
| getItemFromFacetValue(facetValue) { | ||
| let cssClassItem = cx( | ||
| this.props.cssClasses.item, | ||
| {[this.props.cssClasses.active]: facetValue.isRefined} | ||
| ); | ||
| let url = this.getURLFromFacetValue(facetValue); | ||
| let key = facetValue.from + '_' + facetValue.to; | ||
| let handleClick = this.refine.bind(this, facetValue.from, facetValue.to); | ||
| return ( | ||
| <div className={cssClassItem} key={key}> | ||
| <a | ||
| className={this.props.cssClasses.link} | ||
| href={url} | ||
| onClick={handleClick} | ||
| > | ||
| <Template data={facetValue} templateKey="item" {...this.props.templateProps} /> | ||
| </a> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| refine(from, to, event) { | ||
| event.preventDefault(); | ||
| this.setState({ | ||
| formFromValue: null, | ||
| formToValue: null | ||
| }); | ||
| this.props.refine(from, to); | ||
| } | ||
|
|
||
| render() { | ||
| let form = this.getForm(); | ||
| return ( | ||
| <div> | ||
| <div className={this.props.cssClasses.list}> | ||
| {this.props.facetValues.map(facetValue => { | ||
| return this.getItemFromFacetValue(facetValue); | ||
| })} | ||
| </div> | ||
| {form} | ||
| </div> | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| PriceRanges.propTypes = { | ||
| createURL: React.PropTypes.func.isRequired, | ||
| cssClasses: React.PropTypes.shape({ | ||
| active: React.PropTypes.string, | ||
| button: React.PropTypes.string, | ||
| form: React.PropTypes.string, | ||
| input: React.PropTypes.string, | ||
| item: React.PropTypes.string, | ||
| label: React.PropTypes.string, | ||
| link: React.PropTypes.string, | ||
| list: React.PropTypes.string, | ||
| separator: React.PropTypes.string | ||
| }), | ||
| facetValues: React.PropTypes.array, | ||
| labels: React.PropTypes.shape({ | ||
| button: React.PropTypes.string, | ||
| currency: React.PropTypes.string, | ||
| to: React.PropTypes.string | ||
| }), | ||
| refine: React.PropTypes.func.isRequired, | ||
| templateProps: React.PropTypes.object.isRequired | ||
| }; | ||
|
|
||
| PriceRanges.defaultProps = { | ||
| cssClasses: {} | ||
| }; | ||
|
|
||
| module.exports = PriceRanges; | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| var React = require('react'); | ||
|
|
||
| class PriceRangesForm extends React.Component { | ||
| getInput(type) { | ||
| return ( | ||
| <label className={this.props.cssClasses.label}> | ||
| <span className={this.props.cssClasses.currency}>{this.props.labels.currency} </span> | ||
| <input className={this.props.cssClasses.input} ref={type} type="number" /> | ||
| </label> | ||
| ); | ||
| } | ||
|
|
||
| handleSubmit(event) { | ||
| let from = +this.refs.from.value || undefined; | ||
| let to = +this.refs.to.value || undefined; | ||
| this.props.refine(from, to, event); | ||
| } | ||
|
|
||
| render() { | ||
| let fromInput = this.getInput('from'); | ||
| let toInput = this.getInput('to'); | ||
| let onSubmit = this.handleSubmit.bind(this); | ||
| return ( | ||
| <form className={this.props.cssClasses.form} onSubmit={onSubmit} ref="form"> | ||
| {fromInput} | ||
| <span className={this.props.cssClasses.separator}> {this.props.labels.separator} </span> | ||
| {toInput} | ||
| <button className={this.props.cssClasses.button} type="submit">{this.props.labels.button}</button> | ||
|
There was a problem hiding this comment. The only issue with that code is that there won't be any space between the There was a problem hiding this comment. Right, but why do you need one? This should be handled by CSS There was a problem hiding this comment. I'm happy with that; it's the same for the checkboxes in the refinementList widget anyway. |
||
| </form> | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| PriceRangesForm.propTypes = { | ||
| cssClasses: React.PropTypes.shape({ | ||
| button: React.PropTypes.string, | ||
| currency: React.PropTypes.string, | ||
| form: React.PropTypes.string, | ||
| input: React.PropTypes.string, | ||
| label: React.PropTypes.string, | ||
| separator: React.PropTypes.string | ||
| }), | ||
| labels: React.PropTypes.shape({ | ||
| button: React.PropTypes.string, | ||
| currency: React.PropTypes.string, | ||
| separator: React.PropTypes.string | ||
| }), | ||
| refine: React.PropTypes.func.isRequired | ||
| }; | ||
|
|
||
|
|
||
| PriceRangesForm.defaultProps = { | ||
| cssClasses: {}, | ||
| labels: {} | ||
| }; | ||
|
|
||
|
|
||
| module.exports = PriceRangesForm; | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not directly call the
<Form> instead of calling a function? Not sure this is a common pattern in React community.The
<Form>call is only 5 LOC and it would be ovious to read the render() then?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal was to be able to:
getFormmethod separatlygetFormwhen testingrenderThis lets me focus on one thing in each test. In the end I think I did not unit test
getForm(my bad, I might have forgotten).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add the missing test and rebase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should try to avoid mocking when we can, you can always store the expected
<Form>in a variable in your test rather than modifying the implementation to make it easily testable.The least mocking we have in testing render() the better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small detail in the end, if you test getForm() that's ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is maybe due to the fact that most components we always only test the render() method even if that's some boilerplate to add. This way we can just refactor the whole component really easily.
In the end testing a React component output is just asserting some JSX on render(), the underlying implementation should not have to be tested when we can avoid it (sometimes for handlers we need to test it).