-
Notifications
You must be signed in to change notification settings - Fork 54
Feat: additional Layer info on hover previews #1523
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
base: release53
Are you sure you want to change the base?
Feat: additional Layer info on hover previews #1523
Conversation
Co-authored-by: Jonas Hummelstrand <[email protected]>
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| export type PreviewContent = | ||
| | { | ||
| type: 'iframe' | ||
| href: string | ||
| postMessage?: any | ||
| dimensions?: { width: number; height: number } | ||
| } | ||
| | { | ||
| type: 'image' | ||
| src: string | ||
| } | ||
| | { | ||
| type: 'video' | ||
| src: string | ||
| } | ||
| | { | ||
| type: 'script' | ||
| script?: string | ||
| firstWords?: string | ||
| lastWords?: string | ||
| comment?: string | ||
| lastModified?: number | ||
| } | ||
| | { | ||
| type: 'title' | ||
| content: string | ||
| } | ||
| | { | ||
| type: 'inOutWords' | ||
| in?: string | ||
| out: string | ||
| } | ||
| | { | ||
| type: 'layerInfo' | ||
| layerType: SourceLayerType | ||
| text: Array<string> | ||
| inTime?: number | string | ||
| outTime?: number | string | ||
| duration?: number | string | ||
| } | ||
| | { | ||
| type: 'separationLine' | ||
| } | ||
| | { | ||
| type: 'data' | ||
| content: { key: string; value: string }[] | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with this now becoming a public interface this needs to be described more in terms what the Blueprint Developer should expect to get.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, as this is an add on to the existing popupPreview implementation, are there any place where this is already documented, so I can add it there, or du you prefer it as comments in the interface definition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As no previous documentation was found, JSDoc and comments has been added to code.
| inTime?: number | string | ||
| outTime?: number | string | ||
| duration?: number | string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do these properties mean? What's the difference between outTime vs inTime + duration? I'm also somewhat unsure about making this interface this loose & bound to a particular implementation of the hoverscrub preview component/any other component using this infromation - say Presenters' Screen. Any change there would effectively require a change to blueprints integration, which we expect to be at least reasonably stable.
I'm also not a big fan of introducing terms inTime and duration - I think they somewhat overlap with expectedStart and expectedDuration? Are they the same? If that's the case, I think it would make sense to use the same names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are optional values for the "in", "out", "duration" labels, and used for the LayerInfo content, not necessary an expectedStart or expectedDuration calculation.
So either we should add a comment in here that explains what it is, or add it to some documentation, what do you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments for it's use has been added to code.
| out: string | ||
| } | ||
| | { | ||
| type: 'layerInfo' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a layerInfo or more of an "auxiliary Piece info" on a layer of given type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think that calling it layerInfo makes sense, based on what it's used for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments has been added to the code, to explain it's purpose
| | { | ||
| type: 'data' | ||
| content: { key: string; value: string }[] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This interface specifically is very vague for a public-use interface. I think we need to make sure that it's more clear what one can expect/put into here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify what additional specificity you'd like for the data type?
We should definitely dig into what this does and add some documentation, currently I don't know how this feature works, or where it's being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments for it's purpose has been added to code.
| {' '} | ||
| {content.outTime !== undefined && ( | ||
| <> | ||
| <span className="label">{t('OUT')}: </span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should have stylistic capitalization as a part of the Translatable string. Seems like more of a text-transform: uppercase situation? I also suspect that space at the end needs to be an maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As these are labels, should we then decide that "IN", "OUT" and "DURATION" are never translated. Or what are your thoughts and what are we doing other places?
Will look at the
jstarpl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that this is exposing a lot of internal UI mechanics to the Blueprints in a way that ties them to a very specific implementation of the preview mechanics. I see why this can be handy, but I'm worried we may be painting ourselves into a corner for the future, so I think we need to carefully consider documenting what things in the new PreviewContent interface mean, how that information can be used, etc.
|
@jstarpl |
|
@jstarpl Could you please advise on what / if any changes you'd like for this? |
|
@PeterC89 Speaking for the NRK team, we do not block this from being merged. We do have opinions about the way this is implemented and we see the risk of this adding technical debt. NRK is also planning to add functionality in this area which would likely require us to tackle that tech debt and possibly break the public (Blueprint) portion of this API. |
|
@jstarpl We'd be happy to make changes if you can suggest what you'd be keen to get changed? Otherwise we'll get this bought up to date with R53 and merged |
|
@PeterC89 Well, I basically still stand by my original comments above, in summary:
Now, I don't know enough about the User Story behind this feature (and the PR doesn't present any), so I don't know what solution will work in this particular situation to address these issues. One solution that I can think of is keeping the Another alternative could be to also just allow blueprints to have "WebViews" where they could do "direct control HTML" for previews, perhaps better achieving the end goal, but without introducing a tight coupling between the Web UI implementation and Blueprints. Again, I'm speculating a lot here, because I just don't have the context. |
|
Thanks @jstarpl, we'll take this away and have a chat internally tomorrow. The context is that users want to be able to check the spelling and timings of CGs that are 'owned' by an AdLib item on the mini-shelf. The goal was to provide a simple hover mechanism where additional piece information could be seen along with details. I.E. If I cue this AdLib, what is actually going to be cued? |
|
@PeterC89 Yeah, so in my head, the problem is the lack of fidelity our AdLibAction provide in terms of the visibility of side-effects they will produce, stemming from their "Black Box" concept. I think that this is an architectural problem, that maybe the TSC could tackle, of how to handle that "black box" aspect while meeting the needs of the users to have reasonably high fidelity of information and confidence about what will happen once they execute an AdLibAction. My personal stance is that this fidelity should be achieved through machine-readable means, because that then enables information re-use: one UI can display something as a list, while another can display a mini-timeline, yet another headless process can parse that information and take automated actions upon it. But perhaps this is too tall of an order, and we just need HTML viewports that the blueprints can populate. That being said, my personal opinion is that creating another, unplanned Domain Specific Langauge for populating parts of a specific UI delivers neither ease of maintenance, nor data reuse nor sufficient information fidelity (in the long view of things). |
About the Contributor
This PR is made on behalf of BBC
Type of Contribution
This is a feature
Current Behavior
Current it's only possible to use predefined Previews
New Behavior
By adding an optional
additionalPreviewContent?: Array<PreviewContent>inside thePopupPreviewin the Blueprint integration. It's possible to add additionalPreviewContentto HoverPreviews via blueprints.Example:
Testing
Affected areas
The hover preview has been restyled with the "new" Roboto Flex styling.
Mini shelf has an updated 16:9 layout.
Additional content in the hover previews is only affected if the 'additionalPreviewContent' is added in the Blueprints.
Time Frame
This is something BBC would like to have in R53
Status