Settings Component Customization #628
Conversation
| @@ -40,7 +40,7 @@ return ( | |||
| type="checkbox" | |||
| name={visibleColumns[v].id} | |||
| checked | |||
| onClick={onToggle} | |||
| onChange={onToggle} | |||
There was a problem hiding this comment.
Thank you for switching these to the correct events!
|
Thanks! This is a great change. "Settings" is one of the areas we gave the least attention and this really helps a lot! I can hold off until this is ahead of master -- Also, with the TypeScript PR that is coming -- not sure where this one falls in the ideal merge order. Please let me know but this looks good to go and I'd be more than happy to merge soon -- thank you again for all of these great updates! |
|
Do you have a preference between merge/rebase to bring PR branches up to date? I am comfortable with either. Once this is merged, I will definitely rebase the TS branch onto master since it includes some redundant commits. |
|
Either is good with me also. |
Allows removing a built-in setting, e.g.:
const plugin = {
settingsComponentObjects: {
pageSizeSettings: null,
columnChooser: { component: null }
}
};
No test update for SettingsContainer because it has none.
Settings component order is still specified in settingsComponentObjects, and specifying a component there still takes precedence over components; however, the common use case of swapping in a different paging or column component is simplified by treating them as components.
| import _ from 'lodash'; | ||
| import { columnIdsSelector, stylesForComponentSelector } from '../src/selectors/dataSelectors'; | ||
| import * as actions from '../src/actions'; | ||
| import * as selectors from '../src/selectors/dataSelectors'; |
There was a problem hiding this comment.
In retrospect, I should have merged. Regardless, the conflicts were overlapping import lists here. Rather than merge the lists, it seems reasonable to import the full namespace. This will change again with the import refactoring in #618.
There was a problem hiding this comment.
Yea this seems good and sorry if my suggestion of rebasing threw this off.
There was a problem hiding this comment.
Thanks a ton for adding this.
Griddle major version
1.x
Changes proposed in this pull request
componentsnow exports aSettingsComponentskey with the default components.componentis not defined on asettingsComponentObjectsentry, the corresponding key fromcomponents.SettingsComponentsis used.settingsComponentObjectsorcomponents.SettingsComponents, the setting is ignored (instead of an error).settingsComponentObjectsprop can now override default/plugin settings components, as withcomponentsandrenderProperties.order.Plus a few stories proving out scenarios that seem interesting.
Why these changes are made
Hooking into
settingsComponentObjectsdidn't really feel like the other extensibility points, so I started trying alternatives that might align with what I imagine to be the most common scenarios. Specifically:components.SettingsComponent.key = null){ order: n }for the appropriate key(s))components.SettingsComponent.key)Are there tests?
One test; mostly stories showing usage. The
SettingsContainerchanges are most deserving of a test, but there isn't any prior art for testing a container.