-
Notifications
You must be signed in to change notification settings - Fork 64
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
feat(invntory groups): added new group column behind feature flag #1772
feat(invntory groups): added new group column behind feature flag #1772
Conversation
src/store/entities.js
Outdated
invConfig: {}, | ||
sortBy: { | ||
key: 'updated', | ||
direction: 'desc' | ||
} | ||
}; | ||
|
||
export const defaultColumns = () => ([ | ||
export const defaultColumns = (groupsEnabled = useFeatureFlag('hbi.ui.inventory-groups')) => [ |
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 never saw such pattern and not sure it's a good way of calling hooks in the parameters disclosure. Can we just strictly send the boolean parameter with the default value to false?
export const defaultColumns = (groupsEnabled = useFeatureFlag('hbi.ui.inventory-groups')) => [ | |
export const defaultColumns = (groupsEnabled = 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.
And then in the component where you call defaultColumns
, call the hook there and only then pass the parameter to this function.
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.
Yep, that's a better solution
/retest |
@@ -16,13 +18,13 @@ const useColumns = (columnsProp, disableDefaultColumns, showTags, columnsCounter | |||
); | |||
const disabledColumns = Array.isArray(disableDefaultColumns) ? disableDefaultColumns : []; | |||
const defaultColumnsFiltered = useMemo(() => (disableDefaultColumns === true) ? | |||
[] : defaultColumns().filter(({ key }) => | |||
[] : defaultColumns(groupsEnabled).filter(({ key }) => | |||
isColumnEnabled(key, disabledColumns, showTags) | |||
), [disabledColumns, disableDefaultColumns, showTags]); |
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.
We need to add groupsEnabled
to these deps array so that the value is updated when the flag changes.
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.
Ok
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1772 +/- ##
==========================================
- Coverage 68.60% 67.63% -0.97%
==========================================
Files 124 124
Lines 2758 2762 +4
Branches 912 915 +3
==========================================
- Hits 1892 1868 -24
- Misses 866 894 +28
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
/retest |
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.
good job
# [1.7.0](v1.6.0...v1.7.0) (2023-03-03) ### Features * **Inventory table:** Add static groups column ([#1772](#1772)) ([94db33f](94db33f))
🎉 This PR is included in version 1.7.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This pr is adding a Group column to the Inventory table
I had to create a new array of default columns and wrap it in the feature flag