OBPIH-5926 / OBPIH-6206 Delete product source preference records#4534
OBPIH-5926 / OBPIH-6206 Delete product source preference records#4534awalkowiak merged 14 commits intofeature/product-supplier-list-redesignfrom
Conversation
|
|
||
| const afterDelete = () => { | ||
| setValue('defaultPreferenceType', emptyPreferenceType); | ||
| }; |
There was a problem hiding this comment.
not a must have, but I just feel like 27-41 lines could also be included in the hook you've created.
Another not "must have" (because it won't impact the performance at all) is that the emptyPreferenceType could be outside of the component, because its body never changes when a re-render happens (it is not dependent on the component life cycle)
There was a problem hiding this comment.
or forget about this comment - I didn't realize you have different implementations of that depending on whether it is the default preference type or the table.
|
|
||
| const isPreferenceTypeEmpty = !_.some( | ||
| Object.values(_.omit(preferenceTypeData || {}, 'id')), | ||
| ); |
There was a problem hiding this comment.
wouldn't it be cleaner to do it using isEmpty?
const isPreferenceTypeEmpty = _.isEmpty(_.omit(preferenceTypeData || {}, 'id'))There was a problem hiding this comment.
It's not the same.
As an empty preferenceType, we are considering { bidName: '', validityStartDate: '', ... }
isEmpty
In this case, this function returns false because it checks if the object has any keys (it has, but with empty values)some+Object.values
In this case, we are taking only the values of the keys, so it gives us an array like that ['', '', ...] and then we are checking the truthiness of this array, and because of the fact every value in this array is falsy, the whole array is also falsy.
There was a problem hiding this comment.
yep, my bad, didn't understand the context before your explanation.
| if (preferenceTypeData?.id) { | ||
| await productSupplierApi.deleteProductSupplierPreference(preferenceTypeData?.id); | ||
| } | ||
| afterDelete?.(); |
There was a problem hiding this comment.
if we eventually don't call the delete (because of the if above), should we call the afterDelete anyway, or should it be inside the if also?
There was a problem hiding this comment.
yes, because if the row doesn't have the ID, it means it is a new row, so we don't have to make an API call, but we still would like to clear \ remove this row.
There was a problem hiding this comment.
thanks, this is what I wanted to hear
| useDeletePreferenceType.propTypes = { | ||
| preferenceTypeData: PropTypes.shape({}), | ||
| afterDelete: PropTypes.func.isRequired, | ||
| isDefaultPreferenceType: PropTypes.bool, |
There was a problem hiding this comment.
don't you miss onCancel here?
| updatedRows, | ||
| triggerValidation, | ||
| }) => { | ||
| const [selectedRow, setSelectedRow] = useState(null); |
There was a problem hiding this comment.
let's add index to the name (or add a comment above that it is index of a row).
* OBPIH-5926 Add translations * OBPIH-5926 Add function for resetting inputted values * OBPIH-5926 Adjust styling * OBPIH-5926 Create hook for deleting preferenceType * OBPIH-6206 Add new translations * OBPIH-6206 Fix product supplier urls * OBPIH-6206 Export function for resetting default preference type * OBPIH-6206 Fix attributes styling * OBPIH-6206 Add new param to components * OBPIH-6206 Adjust hook for deleting preference types within the table * OBPIH-6206 Use the hook in table * OBPIH-6206 Remove unused property * OBPIH-6206 Split imports into new lines * OBPIH-6206 Add onCancel to propTypes and rename selectedRow variable
3 participants
No description provided.