feat(FEC-10785): add support to remove ui element #613
Conversation
| @@ -82,7 +82,7 @@ class PlayerAreaProvider extends Component { | |||
| _validateComponentData = componentData => { | |||
| // we keep option `container` for backward compatibility. documentation are showing `area` property | |||
| const hasAreaProperty = componentData.container || componentData.area; | |||
| if (!componentData.get || !componentData.presets || !hasAreaProperty) { | |||
| if ((!componentData.get && !componentData.removeComponent) || !componentData.presets || !hasAreaProperty) { | |||
There was a problem hiding this comment.
change the log below accordingly
There was a problem hiding this comment.
Current message also describes our case. If we need to make it more specific then we need to separate the if statement
| @@ -82,7 +82,7 @@ class PlayerAreaProvider extends Component { | |||
| _validateComponentData = componentData => { | |||
| // we keep option `container` for backward compatibility. documentation are showing `area` property | |||
| const hasAreaProperty = componentData.container || componentData.area; | |||
| if (!componentData.get || !componentData.presets || !hasAreaProperty) { | |||
| if ((!componentData.get && !componentData.removeComponent) || !componentData.presets || !hasAreaProperty) { | |||
There was a problem hiding this comment.
I think that because until now our semantics didn't contain action + Component with component suffix we should just call it remove and not removeComponent.
There was a problem hiding this comment.
Today we have also - replaceComponent, beforeComponent, afterComponent. I kept the same semantics
There was a problem hiding this comment.
u r totally right, not sure why I thought the other way around :-) so please disregard.
There was a problem hiding this comment.
but now I have another thought on this.
I don't see removeComponent as same as before|After|replace exactly.
All of the above talk about positional operations, e.g., here to place a component, while remove is not.
Wouldn't it make more sense to re-use replaceComponent and add a get placeholder for a component called None or something that says no component or keep just the replaceComponent and if you have replaceComponent and no get then it means to remove it?
Feels redundant to add more config options and make the object fat where we already have all of the current config options.
There was a problem hiding this comment.
To me it felt more natural to have a new prop called removeComponent.
But I guess with good documentation it is very possible to do it like you suggested
There was a problem hiding this comment.
you know how I personally feel about new APIs or config options - we should always consider if it is necessary as new APIs or not as new ones are an obligation by us and cannot be changed/broken easily after they are committed.
In any case, this is great addition - also need to be able to define this from studio and uiconf which is very useful as we saw in VE.
RoyBregman
dismissed
OrenMe’s stale review
Today we have also - replaceComponent, beforeComponent, afterComponent. I kept the same semantics
Description of the Changes
Until now it was possible to add new component or to add and replace existing components.
There was no official way to remove an existing ui component.
The new way to that now is to use replaceComponent with the existing component and add the special "remove" string in the get property instead of a new component.
solves FEC-10785
changes were done KP repo also - kaltura/kaltura-player-js#460
CheckLists