Feat/UI useredit panel

[olzzon]

This is an internal BBC WIP PR for discussion of concepts for the PropertiesPanel and Useredits.
Be aware:

• The panel is currently implemented as a hack in the Notification Center
• The schema of useredits is currently using a simple "select" tag instead of "Schema...." type, mainly because of styling
• Maybe a "button" type of schema like the grouping array, would be cool to have as a schema, this way e.g. Split/DVE selection could be button with SVG icons on them, instead of a list.
• Buttons can be either text or svg's.

The corresponding Blueprints is in this branch:
https://github.com/bbc/eng-ga-sofie-blueprints/tree/feat/user-edits-properties-panel

[Julusian]

I have a disagreement with how this is done, which is simply that I don't think the properties panel should be using the same list of userEditOperations that we added for the context menu.

I think that the properties panel should be using its own dedicated property, which provides the expected schemas for the panel.
Something like:

interface Part {
  ...

  propertiesPanel?: PropertiesPanelSchema

  ...
}

interface PropertiesPanelSchema {
    /** Common properties to show at the top **/
    headerSchema?: JSONBlob<JSONSchema>
    /** Common properties to show at the bottom **/
    footerSchema?: JSONBlob<JSONSchema>

	/** The json schemas describing the form to display */
	schemas: Record<string, UserEditingSourceLayer>
	/** Current values to populate the form with */
	currentValues: {
		type: string
		value: Record<string, any>
	}
    headerValues?: Record<string, any>
    footerValues?: Record<string, any>
}

As I have said in the comments, I think that reusing the userEditOperations is both making the properties panel ui very complex to handle the displaying and queueing of operations, and will easily result in unexpected behaviour when saving changes in the panel.

There are two key cases I am concerned about, which is that the user triggering the 'ACTION' type of operations in a sequence of A, B, A is not guaranteed to be the same as A, B or even B, A and so we can't reliably condense down the sequence of operations.
And even just executing A, B may not be ok as executing A could remove the ability to execute B.

I don't see a reason that these need to be using the same operations, and while them being separate will likely mean more work for the blueprints dev, it means that the properties panel content will be tailored specifically for the panel.
If they want to have a 'lock part' option in both places, they will have to define it for both, but then they can get better behaviour in both places. If there is an option which affects what other options should be shown/allowed, the json schema for the properties panel could (once we add support for these things to the SchemaForm component) show/hide the appropriate options which are dependent on the others.

[Julusian]

Suggested change

	grouping: clone(userEdit.grouping) as UserEditingGroupingType[] | undefined,
	grouping: clone<UserEditingGroupingType[] | undefined>(userEdit.grouping),

hopefully that satisfies the type error?

The casting you had would ignore some other real type errors

[Julusian]

This operation could be removed now, it existed as a placeholder until real operations were defined

[Julusian]

I think the example operation used __sofie- prefix in the value to ensure it doesnt collide with and blueprint defined operations. I think it would be a good idea to continue that

[Julusian]

Most of this component looks to be duplicated from SchemaFormInPlace.
I think it would be better to change it to be a wrapper component, to be used like:

<StyledSchemaFormWrapper className='' compact width>
  <SchemaFormInPlace ..... />
</StyledSchemaFormWrapper>

I'm not 100% sure what to name this wrapper.

I also want to question whether we need a styled and an unstyled version of this form, especially as this 'styled' version doesn't give any indication of which I should be using where, or the impact of using the wrong one.
I would be curious to see what the affect of the styling is on the other places the schemaform is used. Very little effort has been put into the existing styling, because its only in settings pages

[Julusian]

I disagree with this change. title is set in various schemas to influence the naming of the produced typescript interfaces. This will cause those names to be used instead of user friendly 'ui' names.
eg: https://github.com/nrkno/sofie-timeline-state-resolver/blob/ed2d7a889fbece1e37bc993a7c0c178e220c2575/packages/timeline-state-resolver/src/integrations/httpSend/%24schemas/actions.json#L19-L20

Perhaps it would be ok to do const title = getSchemaUIField(props.schema, SchemaFormUIField.Title) || props.schema.title || props.attr?

[Julusian]

Calling getPendingState twice is not nice, as it is iterating through an array.

I don't think getPendingState even needs to be a function. It is always being called 2 or 3 times each render. And the only other use is inside of the useEffect below, where its going to return the exact same thing as the earlier calls to it

[Julusian]

should this be dependent on props.userEditOperation.isActive? If not, it should be documented so that the next person doesn't add it assuming it was a mistake

[Julusian]

I'm stopping reading this file here, until we have discussed whether this panel should be including everything it is including

[Julusian]

why are the schemas defined with arbitrary string keys, and a looser coupling to sourceLayerType than these values are? They should use the same root key.
I don't think coupling the values to sourceLayerType is a good idea, there is no guarantee that the blueprints will do that internally, they could have two different ways of generating parts which are both 'Camera' parts with completely different schema/value structures.

[Julusian]

I don't think I agree with this behaviour.
On second click of this, it will be executing the operation to change the state to the current value.

And replacing the earlier change can cause some unintended behaviour.
If the user clicks:

1. toggle theme graphic
2. disable all graphics
3. toggle theme graphic (this should enable the theme graphic)

then that will be executed as

1. toggle theme graphic
2. disable all graphics (this now undoes the toggle that was supposed to follow it)

making changes to the queued operations is only ok if we can guarantee that the operations are not dependent on each other.
but also I understand why you have done that, as we don't want to build a queue of hundreds of operations because the user was clicking around while figuring out what they are trying to do.