Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(FEC-10785): add support to remove ui element #460

Merged
merged 8 commits into from
Jun 27, 2021

Conversation

RoyBregman
Copy link
Contributor

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.

  • Flow - KPUIComponent was separated to KPUIAddComponent and KPUIRemoveComponent
  • UIWrapper was added a new api - removeComponent(component: KPUIRemoveComponent): Function
  • getUIComponents handling was changed accordingly

solves FEC-10785
changes were done also in ui repo - kaltura/playkit-js-ui#613

CheckLists

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • test are passing in local environment
  • Travis tests are passing (or test results are not worse than on master branch :))
  • Docs have been updated

|};

declare type KPUIRemoveComponent = {|
removeComponent?: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
removeComponent?: string,
removeComponent: string,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right!

};
replaceComponent?: string,
presets: Array<string>,
area: string
Copy link
Contributor

Choose a reason for hiding this comment

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

move them up like before (mandatory first)

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

let replaceComponent: KPUIAddComponent = {
label: `Remove_${component.removeComponent}`,
get: Components.Remove,
presets: component.presets.concat(),
Copy link
Contributor

Choose a reason for hiding this comment

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

why concat is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To create a copy of the array

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. why copy?
  2. i think [...component.presets] is clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1.Copy because it seemed to me more correct. But not sure actually why not.
2. Because I'm old and I wrote it 1999 style

yairans
yairans previously approved these changes Jun 23, 2021
yairans
yairans previously approved these changes Jun 24, 2021
OrenMe
OrenMe previously approved these changes Jun 24, 2021
yairans
yairans previously approved these changes Jun 24, 2021
Yuvalke and others added 3 commits June 27, 2021 08:53
Issue: share configured ui from kaltura player.
Solution: handle this config as a plugin config.
…a/kaltura-player-js into FEC-10785-removeComponent

# Conflicts:
#	flow-typed/types/ui-component.js
@RoyBregman RoyBregman dismissed stale reviews from yairans and OrenMe via ff8175d June 27, 2021 10:07
@Yuvalke Yuvalke closed this Jun 27, 2021
@Yuvalke Yuvalke reopened this Jun 27, 2021
@RoyBregman RoyBregman merged commit fd2172c into master Jun 27, 2021
@RoyBregman RoyBregman deleted the FEC-10785-removeComponent branch June 27, 2021 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants