global: add prefix and suffix props #106
Conversation
There was a problem hiding this comment.
Thanks for the changes, and also for having updated the demo and documentation.
If you see the screenshot in the README of this repo, you might have it also for the Sort components, there are 3 of them. I would propose to add this suffix and prefix also to them.
src/lib/components/Count/Count.js
Outdated
| return ( | ||
| <ShouldRender condition={!loading && totalResults > 0}> | ||
| {this.renderSpanWithMargin(prefix, 'right')} |
There was a problem hiding this comment.
Prefix or Suffix might be not defined, so the extra margin should not appear. I would change both to:
{prefix && this.renderSpanWithMargin(prefix, 'right')}
WDYT?
| return ( | ||
| <ShouldRender | ||
| condition={!loading && totalResults > 0 && currentSize !== -1} | ||
| > | ||
| {this.renderSpanWithMargin(prefix, 'right')} |
| render() { | ||
| const { loading, currentSize, totalResults } = this.props; | ||
| const { loading, currentSize, totalResults, prefix, suffix } = this.props; |
There was a problem hiding this comment.
May I suggest a change that IMHO it fits a bit better the purpose of the prefix/suffix props without making it too specific? As I understood, please correct me if I am wrong, the end goal is to apply a custom label that wraps the ResultsPerPage component. I would go then for a label prop that can be a function like this:
ResultsPerPage label={(cmp) => `Show {cmp} results per page`} />then in the render function of the ResultsPerPage component you could write something similar to this:
render() {
const { loading, currentSize, totalResults, label } = this.props;
...
{label && label(this.renderElement(currentSize, this.options, this.onChange))}
...
}
| {prefix && this.renderSpanWithMargin(prefix, 'right')} | ||
| {this.renderElement(currentSize, this.options, this.onChange)} | ||
| {suffix && this.renderSpanWithMargin(suffix)} | ||
| <span style={({ marginLeft: size }, { marginRight: size })}> |
There was a problem hiding this comment.
I think this styling should go to the application that uses this component so as to keep the components here with minimal styling :)
src/lib/components/Sort/Sort.js
Outdated
| this.onChange | ||
| )} | ||
| {suffix && this.renderSpanWithMargin(suffix)} | ||
| <span style={({ marginLeft: size }, { marginRight: size })}> |
src/lib/components/SortBy/SortBy.js
Outdated
| {prefix && this.renderSpanWithMargin(prefix, 'right')} | ||
| {this.renderElement(currentSortBy, this.options, this.onChange)} | ||
| {suffix && this.renderSpanWithMargin(suffix)} | ||
| <span style={({ marginLeft: size }, { marginRight: size })}> |
| {prefix && this.renderSpanWithMargin(prefix, 'right')} | ||
| {this.renderElement(currentSortOrder, this.options, this.onChange)} | ||
| {suffix && this.renderSpanWithMargin(suffix)} | ||
| <span style={({ marginLeft: size }, { marginRight: size })}> |
docs/docs/components/count.md
Outdated
|
|
||
| An optional string to be displayed after the number of results. | ||
| An optional function to wrap the component with a prefix and suffix string. <br /> | ||
| E.g. `label={(val) => <> Found {val} results</>} />` |
There was a problem hiding this comment.
I would probably change the val to cmp just to indicate that the passed argument is the component not just a value. This would make clear that the return value of the label function needs to be a component and cannot be a string :) Please do the corresponding changes to the other places :)
Closes #104
Example:
