-
Notifications
You must be signed in to change notification settings - Fork 55
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?
Changes from all commits
9eff487
7eb12a5
a497576
0fd2a0d
3f26c87
092fd52
f526397
6fece09
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,15 @@ | ||
import { SplitsContentBoxContent, SplitsContentBoxProperties } from './content.js' | ||
import { SourceLayerType, SplitsContentBoxContent, SplitsContentBoxProperties } from './content.js' | ||
import { NoteSeverity } from './lib.js' | ||
import { ITranslatableMessage } from './translations.js' | ||
|
||
export interface PopupPreview<P extends Previews = Previews> { | ||
name?: string | ||
preview?: P | ||
warnings?: InvalidPreview[] | ||
/** | ||
* Add custom content preview content | ||
*/ | ||
additionalPreviewContent?: Array<PreviewContent> | ||
} | ||
export type Previews = TablePreview | ScriptPreview | HTMLPreview | SplitPreview | VTPreview | BlueprintImagePreview | ||
|
||
|
@@ -19,6 +23,55 @@ export enum PreviewType { | |
BlueprintImage = 'blueprintImage', | ||
} | ||
|
||
// The PreviewContent types are a partly replica of the types in PreviewPopUpContext.tsx | ||
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 | ||
Comment on lines
+63
to
+65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do these properties mean? What's the difference between I'm also not a big fan of introducing terms There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
| { | ||
type: 'separationLine' | ||
} | ||
| { | ||
type: 'data' | ||
content: { key: string; value: string }[] | ||
} | ||
Comment on lines
+70
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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? |
||
|
||
Comment on lines
+27
to
+74
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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? |
||
interface PreviewBase { | ||
type: PreviewType | ||
} | ||
|
Uh oh!
There was an error while loading. Please reload this page.
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.