Entity type editor – Show correct properties in property selector, add edit bar, and minor changes

[nathggns]

🌟 What is the purpose of this PR?

Next stage in entity type editor – see below.

🔗 Related links

• Asana task (internal)

🚫 Blocked by

N/A

🔍 What does this change?

Functionality changes:

• Populate property type selector with existing property types, hide property types already added
• Ensure the property type you click on is the one inserted and displayed
• Temporarily remove inoperable property menu buttons
• Ensure property menu shows the correct version number
• Edit bar – including operable discard changes button
• Add JSON object type for expected values chips
• Tooltip with full path for property type in the property type menu

Code changes:

• New useInitTypeSystem hook – and init the type system on the view entity type page
• useEntityType hook now takes on onCompleted param, which we use to reset the state of the editor whenever the entity type loads
• We no longer use router.asPath to resolve the entity type – this fixes a bug where hash or query params breaks finding the entity type
• The state management of changes / UI mode has been moved to simplify the UI
• All property types are now fetched on load and provided via a context to enable the property selector to display them
• New dataTypeNames to show the human readable name of data type chips
• The new property type menu no longer inserts a fake property type – this is in preparation for making it actually work

📜 Does this require a change to the docs?

N/A

⚠️ Known issues

🐾 Next steps

🛡 What tests cover this?

❓ How to test this?

1. Create an entity type in the new entity type form
2. Ensure you can select property types from the property type selector and that the edit bar appears
3. Ensure you can remove property types
4. Ensure discard changes on the edit bar works

[lgtm-com]

This pull request introduces 1 alert when merging 739444a into 6303196 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 1 alert when merging 509121d into 074ed5d - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[kachkaev]

The diff looks clean – I've left a couple of optional code style suggestions

[kachkaev]

Optional, for all similar cases: it'd useful to use explicit return statements in all React components instead of this arrow function shortcut. If we need to add some hook later, we won't end up with a crazy diff.

[nathggns]

Is there an autofixable lint rule for this? I tend to agree (from an "optimise for change" perspective) – although not for diff reasons because you can just hide whitespace in the diff, just for reducing friction of adding a hook. You could create a ticket?

[kachkaev]

you can just hide whitespace in the diff

That won't help if JSX is wide enough to be re-formatted.

I created https://app.asana.com/0/1200211978612931/1203132574912430/f

[kachkaev]

I remember us chatting about potential removal of FunctionComponent in code, but if we go for it, we should probably do so in some organised way. I'd stick with FunctionComponent for consistency for now, especially for exportable components. Defining type FooBarProps separately can also contribute to overall consistency. I'm not strongly opinionated about any of the styles, just suggesting to keep code rhythm as consistent as possible.

[kachkaev]

Suggested change

	const OntologyChipForwardRef = forwardRef(OntologyChip);
	export { OntologyChipForwardRef as OntologyChip };
	export const OntologyChip = forwardRef(RenderOntologyChip);

Suggested change

	const OntologyChipForwardRef = forwardRef(OntologyChip);
	export { OntologyChipForwardRef as OntologyChip };
	export const OntologyChip = forwardRef(renderOntologyChip);

etc.

ForwardRefRenderFunction is not a React component because it has two args instead of one. It's useful to call ref forwarding functions distinctly to avoid accidental misuse or confusion.

[nathggns]

What you're suggesting messes up the name in dev tools, hence this approach (which is already used elsewhere in the project).

We want the dev tools to look like this

What you're suggesting makes it look like this

[nathggns]

It's also worth saying forwardRef is a stop gap solution. They plan to remove it. reactjs/rfcs#107

[kachkaev]

Oh right I did not know about that effect for the dev tools. I think I usually just call forwardRef inline, but I'm beginning to forget that. Need to write more react code 😅