-
Notifications
You must be signed in to change notification settings - Fork 25
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
Enable custom configuration settings for the data table manager #2851
Enable custom configuration settings for the data table manager #2851
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 1e74e48 The changes in this PR will be included in the next version bump. This PR includes changesets to release 96 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This PR is missing a Jira ticket reference in the title or description. |
|
||
const customSettings = [ | ||
{ | ||
id: 'customSettings1', |
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 payload can take as much settings as the user wants to access in the nested rows.
@@ -350,6 +477,8 @@ storiesOf('Components|DataTable', module) | |||
'showColumnManagerConfirmationButtons', | |||
false | |||
); | |||
// TODO - remove when nexted rows are implemented | |||
const debug = boolean('logCustomSettings', false); |
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 solely for storybook and testing purpose. Will be helpful when implementing the nested rows. It is currently a toggle in storybook to help see the data passed in the customSettingsPayload
@ddouglasz Thanks for doing this. I took a quick glance at the code and noticed a couple of things that might be crucical for adapting in MC products app. Please let me know your thoughts or any alternative suggestion to resolve this.
|
| `customSettings['key'].customComponent` | `ReactNode` | | | A component added to the settings interface to provide additional configuration for the data table setting | | ||
| `customColumnManager.disableColumnManager` | `bool` | | `true` | Set this to `false` to show the column settings panel option. | | ||
| `customColumnManager.visibleColumnKeys` | Array of `string` | | | The keys of the visible columns. | |
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.
@ddouglasz Could you also describe the prop customColumnManager
?
I understand that the propcustomSettings
is used to display customized options under the table settings dropdown and it also takes in customSettings['key'].customComponent
which renders a react component when user selects the corresponding option.
When do we have to use customColumnManager
? I checked the storybook from the preview link and I can't find where the component is rendered (for eg: I see this line columnManagerLabel: 'Custom Column Manager',
but not sure where it is displayed in the UI
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.
Thank you @jaikumar-tj ,
Nice catch! I have updated the readme here to describe it's use.
I missed the proper cleanup of the property. It is only meant for the already existing data table managers. Remember from our last feedback, we also want to have the option of overriding the default manager labels. When this value is provided, it overrides the label of the default manager labels.
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.
@ddouglasz
Thanks for updating readme. Here is my overall understanding and let me know if it is correct
What about customColumnManager
? When do we have to use that? You mentioned that "It is only meant for the already existing data table managers" but I'm not sure if you are referring to this prop or the one about overriding label.
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.
@ddouglasz Feel free to merge the PR. I think my doubts might be clarified once we start implementing this new changes in the product list.
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.
Sure! I pushed the change from our meeting yesterday. Thanks for the review. Will be happy to align with you at any point, if you have more questions.
6af7dcf
Thank you.
Summary
Enable custom configuration settings for data table amanager
Description
We want to be able to allow users to create their desired table configurations using the data table manager. This will allow granular configurations for the users as they can now create more settings, beyond the currently existing columns settings and display settings.
Reference: #2687 (comment)