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!: Editable fields as JSON schema #1005

Merged

Conversation

sbaudlr
Copy link
Contributor

@sbaudlr sbaudlr commented Aug 25, 2023

This PR has been opened by SuperFly.tv on behalf of EVS Broadcast Equipment (henceforth EVS).

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Feature (Closes #857).

What is the current behavior? (You can also link to an open issue here)

Editable fields use the deprecated config manifest format.

What is the new behavior (if this is a feature change)?

Editable fields now use JSON-schema.

Other information:

The ability to change the content of editable fields from the inspector in the shelf has been removed, as this is seemingly unused at the moment. It may be possible to use the components that have been developed for displaying JSON schemas in the config UIs for this purpose if desired, but this has not been tested.

Status

  • Code documentation for the relevant parts in the code have been added/updated by the PR author
  • The functionality has been tested by the PR author
  • The functionality has been tested by NRK

@Julusian Julusian requested review from jstarpl and a team August 25, 2023 11:32
@Julusian Julusian added the Contribution from EVS Contributions sponsored by EVS Broadcast Equipment (evs.com) label Aug 25, 2023
@Julusian
Copy link
Member

I think that this could do with some cleanup to be done too. This should mean that the ConfigManifestEntry and related types from blueprints-integration can be removed, as well as the ui components which draw them. I think that cleanup should be done in this PR, as this does make it all dead code.

I do not know enough about the inspector and potential usages of it to say whether it is ok to remove the editable fields section in the ui. But I think it would be better to leave the current ui bits in place, and to replace some of the inside of renderConfigFields with something like <span>Not implemented!</span>, so that when someone tries to use the editableFields property next they don't have to spend time trying to figure out why it isn't working and are instead reminded that it isnt implemented.

@sbaudlr
Copy link
Contributor Author

sbaudlr commented Aug 25, 2023

I think that this could do with some cleanup to be done too. This should mean that the ConfigManifestEntry and related types from blueprints-integration can be removed, as well as the ui components which draw them. I think that cleanup should be done in this PR, as this does make it all dead code.

I do not know enough about the inspector and potential usages of it to say whether it is ok to remove the editable fields section in the ui. But I think it would be better to leave the current ui bits in place, and to replace some of the inside of renderConfigFields with something like <span>Not implemented!</span>, so that when someone tries to use the editableFields property next they don't have to spend time trying to figure out why it isn't working and are instead reminded that it isnt implemented.

I'll remove the unused things and look into doing something with the inspector next week.

@jstarpl jstarpl merged commit 819e41e into nrkno:release51 Sep 29, 2023
31 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟 Code improvement Contribution from EVS Contributions sponsored by EVS Broadcast Equipment (evs.com) ✨ enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants