Skip to content
This repository has been archived by the owner on Nov 13, 2024. It is now read-only.

DEX-1270 remove id from optionEditor #412

Merged
merged 7 commits into from
Jul 7, 2022
Merged

DEX-1270 remove id from optionEditor #412

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b1f5534
DEX-1270 remove id from optionEditor
Atticus29 Jun 30, 2022
77c5ee3
DEX-1120 respond to code review feedback
Atticus29 Jul 5, 2022
2ffec49
DEX-1120 respond to code review feedback part 2
Atticus29 Jul 6, 2022
b2c0f77
DEX-1120 respond to code review feedback part 3
Atticus29 Jul 7, 2022
ed875d1
DEX-1120 only show tooltip when not valid
Atticus29 Jul 7, 2022
75e6ad4
DEX-1120 DRY up the ButtonWithTooltip props
Atticus29 Jul 7, 2022
0f31a48
DEX-1120 respond to lodash improvement feedback
Atticus29 Jul 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/pages/fieldManagement/settings/saveField/OptionEditor.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import React from 'react';
import { FormattedMessage } from 'react-intl';
import { v4 as uuid } from 'uuid';
import FormControl from '@material-ui/core/FormControl';
import DialogContent from '@material-ui/core/DialogContent';
import DialogActions from '@material-ui/core/DialogActions';
Expand All @@ -21,7 +20,7 @@ export default function OptionEditor({
}) {
const options = value || [];
const displayedOptions =
options.length > 0 ? options : [{ label: '', value: '', id: 6 }];
options.length > 0 ? options : [{ label: '', value: '' }];

return (
<StandardDialog
Expand All @@ -32,7 +31,7 @@ export default function OptionEditor({
<DialogContent style={{ minWidth: 200 }}>
{displayedOptions.map((option, optionIndex) => {
const otherOptions = options.filter(
o => o.id !== option.id,
o => o.value !== option.value,
);
const showDeleteButton = displayedOptions.length !== 1;
return (
Expand All @@ -42,7 +41,7 @@ export default function OptionEditor({
flexDirection: 'column',
marginBottom: 12,
}}
key={option.id}
key={option.value}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option values and/or labels cannot be guaranteed to be unique within this component. If a user clicks "Add Another Option" several times, each option looks exactly the same: [{ label: '', value: '' }, { label: '', value: '' }, // etc.]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, "value" is not a stable identifier for a key because whenever an option's value is updated, the key changes. This causes the value input to lose focus.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm...what would you recommend in this case? Just generate a uuid?

Copy link
Contributor Author

@Atticus29 Atticus29 Jul 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option values and/or labels cannot be guaranteed to be unique within this component. If a user clicks "Add Another Option" several times, each option looks exactly the same: [{ label: '', value: '' }, { label: '', value: '' }, // etc.]

I think that this will at least be partially ameliorated by Jon's back end work on DEX-1270 (this makes me realize that I mis-typed and this whole FE PR is for ticket DEX-1120).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be partially ameliorated because we will be able to know that the values from the database are unique. However, while values are being added and edited within the dialog, it will always be possible for multiple value fields to be the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once the values from the database are unique, it might be sufficient to add a timestamp property to objects created on the frontend ie. { label, value, created: Date.now() }. obviously the backend objects wouldn't have these but you could use key={o?.created || o?.value}

Copy link
Contributor

@brmscheiner brmscheiner Jul 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, "value" is not a stable identifier for a key because whenever an option's value is updated, the key changes. This causes the value input to lose focus.

for this to be resolved we would need to make sure the value used in the keys was the backend value, not the frontend's input value that changes as typed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok... with the most recent changes, I believe that the only way that two options would have the same key would be if 1) they were migrated and 2) they had the same exact value.

I would argue that the behavior of deletion of any one of these resulting in the deletion of all of them is a feature rather than a bug, because it helps improve the usability of the product.

>
<div style={{ display: 'flex', alignItems: 'center' }}>
<Text variant="subtitle2" id="OPTION" />
Expand Down Expand Up @@ -98,7 +97,6 @@ export default function OptionEditor({
{
label: '',
value: '',
id: uuid(),
},
]);
}}
Expand Down