made it possible to change filter placeholder text #780
Conversation
Saldivarcher
changed the title
Saldivarcher
changed the title
stories/index.tsx
Outdated
| @@ -767,6 +767,14 @@ storiesOf('Griddle main', module) | |||
| </Griddle> | |||
| ) | |||
| }) | |||
| .add('with Filter place-holder', () => { | |||
There was a problem hiding this comment.
Can you move this down into a new Filter section of stories, similar to the story for settingsToggle?
There was a problem hiding this comment.
Should I also move all the filter examples from the Griddle Main section, to this section?
src/core/initialState.js
Outdated
| @@ -40,6 +40,7 @@ export default { | |||
| textProperties: { | |||
| next: 'Next', | |||
| previous: 'Previous', | |||
| settingsToggle: 'Settings' | |||
| settingsToggle: 'Settings', | |||
| filterPlaceHolder: 'Filter' | |||
There was a problem hiding this comment.
- Can we rename this as
filterPlaceholdersince the attribute isplaceholdernotplaceHolder? - Maybe keep the list alphabetized?
src/components/Filter.js
Outdated
| @@ -5,7 +5,8 @@ class Filter extends Component { | |||
| static propTypes = { | |||
| setFilter: PropTypes.func, | |||
| style: PropTypes.object, | |||
| className: PropTypes.string | |||
| className: PropTypes.string, | |||
| text: PropTypes.string | |||
There was a problem hiding this comment.
Would it make sense to rename this prop as placeholder, to align with the textProperties key and to make explicit how the value will be used?
Bonus points if you add a trailing comma.
|
@ryanlanciaux also, this includes a |
|
Changes look good to me. I'm ambivalent on relocating the other Filter story, as it really belongs in a Redux section. |
|
@dahlbyk @ryanlanciaux I don't think that's a bad idea to put it into a redux section, along with the custom column chooser within the settings story. |
|
Were these changes good, or should I fix them @dahlbyk? 😸 |
|
Sorry for the delay, @miguelsaldivar. All the maintainers are stretched pretty thin, but we do appreciate the contribution! |
|
No no, it's fine don't even worry 😄, I'll look for some other bugs I can fix to help you guys out! |
Griddle major version
1.11.0
Changes proposed in this pull request
Fixed one of the issues regarding #774, which is an easy change to filter place holder text
Why these changes are made
To be able to change the place holder text without creating a new component.
Are there tests?
Ran
npm run testand it passed all tests.