feat(metadata-views): Support adding removing values #2284
Conversation
manishPh
force-pushed
the
jsonOpsMDView
branch
from
ce1de24
to
1cab5fb
Compare
|
|
||
| default: | ||
| return value; | ||
| if (type === FIELD_TYPE_INTEGER && !isNil(value)) { |
There was a problem hiding this comment.
using isNil() to separate null/undefined with values like 0
| @@ -258,17 +259,18 @@ class MetadataBasedItemList extends React.Component<Props, State> { | |||
| return cellData; | |||
| } | |||
| const { type, value, options = [] } = field; | |||
| const shouldShowEditIcon = isCellEditable && isString(type); | |||
There was a problem hiding this comment.
Making sure data type of metadata field is valid, so that we can show edit icon and corresponding <MetadataField /> for that type.
| <IconWithTooltip | ||
| type={EDIT_ICON_TYPE} | ||
| tooltipText={<FormattedMessage {...messages.editLabel} />} | ||
| onClick={() => this.handleEditIconClick(columnIndex, rowIndex, value)} | ||
| /> | ||
| )} | ||
| {isCellBeingEdited && valueBeingEdited && ( |
There was a problem hiding this comment.
Show edit icon even for the case where value is not present to start with; i.e. for JSON patch add operation
There was a problem hiding this comment.
Is there a snapshot test for this change?
There was a problem hiding this comment.
@sahiga there is but its difficult to test react-virtualized <AutoSizer /> component with enzyme because of issues like this. The other alternative I found out was to export one more component from the same file which is wrapped in <VirtualScroll /> as mentioned here so that we can test the functionality but I guess its too complicated for a snapshot test. I have instead relied on unit testing other functionality from the component.
| ancestor_folder_id: 0, | ||
| }; | ||
|
|
||
| This function will return ['field_1', 'field_2', 'field_3'] |
There was a problem hiding this comment.
['field_1', 'field_2', 'field_3'] will become the columns in the grid view.
| }); | ||
| const queryFields = this.getMetadataQueryFields(); | ||
|
|
||
| const fields = queryFields.map((queryField: string) => { |
There was a problem hiding this comment.
Iterate over the fields mentioned in the query instead of fields returned in the response. The metadata_queries API will return only the existing fields.
| @@ -169,6 +169,8 @@ describe('features/metadata-based-view/MetadataBasedItemList', () => { | |||
| ${FIELD_TYPE_FLOAT} | ${'123.456'} | ${123.456} | |||
| ${FIELD_TYPE_MULTISELECT} | ${['Yes', 'No']} | ${['Yes', 'No']} | |||
| ${FIELD_TYPE_STRING} | ${'str'} | ${'str'} | |||
| ${FIELD_TYPE_STRING} | ${undefined} | ${undefined} | |||
| ${null} | ${'some value'} | ${'some value'} | |||
There was a problem hiding this comment.
Should we also test the cases in which the type is FIELD_TYPE_INTEGER or FIELD_TYPE_FLOAT, but the value is null or undefined?
There was a problem hiding this comment.
The additional two cases cover the function behavior where it returns value when either value or type doesn't exist. Checking for more types for the same result may not add additional testing.
| <IconWithTooltip | ||
| type={EDIT_ICON_TYPE} | ||
| tooltipText={<FormattedMessage {...messages.editLabel} />} | ||
| onClick={() => this.handleEditIconClick(columnIndex, rowIndex, value)} | ||
| /> | ||
| )} | ||
| {isCellBeingEdited && valueBeingEdited && ( |
There was a problem hiding this comment.
Is there a snapshot test for this change?
manishPh
force-pushed
the
jsonOpsMDView
branch
from
1cab5fb
to
f54d535
Compare
This PR enables metadata views to add/remove metadata values in addition to replace (PUT) them.