feature/OC-718/mappings-code-editor #328
Conversation
pwa/src/templates/templateParts/mappingForm/MappingFormTemplate.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| switch (name) { | ||
| case "mapping": | ||
| setEditMappingInEditor(!editMappingInEditor); |
There was a problem hiding this comment.
Whenever toggling a state, we should hook into the current state using the callback function. Right now, this could break during the React lifecycle.
e.g.
setEditMappingInEditor((currentMappingInEditor) => !currentMappingInEditor);
This way you can be 100% sure you're inverting the right value.
| @@ -80,19 +104,97 @@ export const MappingFormTemplate: React.FC<MappingFormTemplateProps> = ({ mappin | |||
| basicFields.forEach((field) => setValue(field, mapping[field])); | |||
|
|
|||
| const newMapping = Object.entries(mapping.mapping).map(([key, value]) => ({ key, value })); | |||
| setMapping(newMapping); | |||
| setMappingKeyValue(newMapping); | |||
| setMappingCodeEditor(JSON.stringify(mapping.mapping.length ? mapping.mapping : {}, null, 2)); | |||
There was a problem hiding this comment.
I'm thinking we could maybe get rid of the mappingCodeEditor, unsetCodeEditor, ... altogether by making one generic function that simply returns the stringified value of the mapping.
E.g. (dummy code):
const getStringifiedCode = (type: "mapping" | "unset" | "cast") => { // duplicating the type here; we should make an actual Type
return JSON.stringify(mapping[type].length ? mapping[type] : {}, null, 2);
}
Now getStringifiedCode("cast") would hold the value of castCodeEditor, removing the need to keep track of the state (and requiring to JSON.stringify every time we need the value.
| const [mappingKeyValue, setMappingKeyValue] = React.useState<any[]>([]); | ||
| const [mappingCodeEditor, setMappingCodeEditor] = React.useState<string>(""); | ||
| const [editMappingInEditor, setEditMappingInEditor] = React.useState<boolean>(false); | ||
|
|
||
| const [unsetKeyValue, setUnsetKeyValue] = React.useState<any[]>([]); | ||
| const [unsetCodeEditor, setUnsetCodeEditor] = React.useState<string>(""); | ||
| const [editUnsetInEditor, setEditUnsetInEditor] = React.useState<boolean>(false); | ||
|
|
||
| const [castKeyValue, setCastKeyValue] = React.useState<any[]>([]); | ||
| const [castCodeEditor, setCastCodeEditor] = React.useState<string>(""); | ||
| const [editCastInEditor, setEditCastInEditor] = React.useState<boolean>(false); | ||
|
|
||
| const [isOpenMapping, setIsOpenMapping] = React.useState<boolean>(!mapping); | ||
| const [isOpenUnset, setIsOpenUnset] = React.useState<boolean>(!mapping); | ||
| const [isOpenCast, setIsOpenCast] = React.useState<boolean>(!mapping); |
There was a problem hiding this comment.
(See other comment about removing mappingCodeEditor, unsetCodeEditor and castCodeEditor).
It might be possible to simplify this logic. Basically, we want to keep track of three things (I think):
- Which one (of three) types of mappings are open;
- Which one (of three) types is being edited as an JSON;
- The value of each one.
Currently, we have 12 states to keep track of this. Not necessarily an issue, however, it might be worthwhile to check if we can reduce that number.
Possible (dummy!) solution:
type TMappingTypeState = {
open: boolean;
editAsJSON: boolean;
value: string;
}
interface IMappingState {
mapping: TMappingTypeState,
unset: TMappingTypeState,
cast: TMappingTypeState,
}
const [mappingsState, setMappingsState] = React.useState<IMappingState>({ ...default state });
// I think we could run all further logic from this one state, making it easier to be managed.
| //Mappings | ||
| React.useEffect(() => { | ||
| if (mappingCodeEditor === "") return; | ||
|
|
||
| try { | ||
| JSON.parse(mappingCodeEditor); | ||
| } catch (e) { | ||
| return; | ||
| } | ||
|
|
||
| const jsonMapping = mappingCodeEditor && JSON.parse(mappingCodeEditor); | ||
|
|
||
| const newMapping = Object.entries(jsonMapping).map(([key, value]) => ({ key, value })); | ||
|
|
||
| if (!_.isEqual(mappingKeyValue, newMapping)) setMappingKeyValue(newMapping); | ||
| }, [mappingCodeEditor]); | ||
|
|
||
| React.useEffect(() => { | ||
| if (!watchMapping?.length) return; | ||
|
|
||
| const test = Object.assign({}, ...watchMapping.map(({ key, value }: any) => ({ [key]: value }))); | ||
|
|
||
| setMappingCodeEditor(JSON.stringify(test, null, 2)); | ||
| }, [watchMapping]); | ||
|
|
||
| //Unset | ||
| React.useEffect(() => { | ||
| if (unsetCodeEditor === "") return; | ||
|
|
||
| try { | ||
| JSON.parse(unsetCodeEditor); | ||
| } catch (e) { | ||
| return; | ||
| } | ||
|
|
||
| const jsonUnset = unsetCodeEditor && JSON.parse(unsetCodeEditor); | ||
|
|
||
| const newUnset = Object.entries(jsonUnset).map(([key, value]) => ({ key, value })); | ||
|
|
||
| if (!_.isEqual(unsetKeyValue, newUnset)) setUnsetKeyValue(newUnset); | ||
| }, [unsetCodeEditor]); | ||
|
|
||
| React.useEffect(() => { | ||
| if (!watchUnset?.length) return; | ||
|
|
||
| const test = Object.assign({}, ...watchUnset.map(({ key, value }: any) => ({ [key]: value }))); | ||
|
|
||
| setUnsetCodeEditor(JSON.stringify(test, null, 2)); | ||
| }, [watchUnset]); | ||
|
|
||
| //Cast | ||
| React.useEffect(() => { | ||
| if (castCodeEditor === "") return; | ||
|
|
||
| try { | ||
| JSON.parse(castCodeEditor); | ||
| } catch (e) { | ||
| return; | ||
| } | ||
|
|
||
| const jsonCast = castCodeEditor && JSON.parse(castCodeEditor); | ||
|
|
||
| const newCast = Object.entries(jsonCast).map(([key, value]) => ({ key, value })); | ||
|
|
||
| if (!_.isEqual(castKeyValue, newCast)) setCastKeyValue(newCast); | ||
| }, [castCodeEditor]); |
There was a problem hiding this comment.
We're duplicating the same logic three times here. When you've implemented the other comment about simplifying the state, this might be easily simplified as well. If not, maybe we can try and pair-program a solution.
| <> | ||
| <div onClick={(e) => handleSwitchEditor(e, "mapping")}> | ||
| <Link icon={<FontAwesomeIcon icon={faEdit} />} iconAlign="start"> | ||
| {t(!editMappingInEditor ? "Edit as JSON" : "Edit as Key-Value")} |
There was a problem hiding this comment.
I think this button is a bit out-of-place. I've tried to replace it with Tabs, but that didn't really work out either. Maybe using the ActionButton (needs some refactoring, to also allow these actions and icons), resulting in something like (before refactoring):
…/mappings-code-editor
No description provided.