[Breaking] Align all text-style related types to better match the use-cases of workspace #383
Conversation
srares76
requested review from
brapoprod,
abdelhalimkhouas,
alexandraFlavia9 and
pietervp
as code owners
Coverage ReportCoverage report can be checked at https://cdnepgrafxstudiodev.azureedge.net/sdk/coverage/383/coverage.html Use PR sdk packageTarball can be downloaded from https://cdnepgrafxstudiodev.azureedge.net/sdk/dev-packages/383/studio-sdk.tgz To use in local project, change package.json to use local tarball "dependencies": {
"@chili-publish/studio-sdk": "https://cdnepgrafxstudiodev.azureedge.net/sdk/dev-packages/383/studio-sdk.tgz"
} |
srares76
changed the title
There was a problem hiding this comment.
One missing property & some confusion I may have spread on one interface.
Just putting those here to make it more clear:
Dart ParagraphStyleDto
Dart CharacterStyleDto
Dart SelectedTextStyleDto
We can still decide to keep properties optional by default but in engine world they are actually required for our 3 models.
| export interface SelectedTextStyle { | ||
| paragraphStyleId?: string; | ||
| export type BaseTextStyle = { | ||
| alignToBaseLine?: boolean; |
There was a problem hiding this comment.
| alignToBaseLine?: boolean; | |
| alignToBaseLine: boolean | null; |
Same for all properties below, they are actually required & nullable properties.
|
|
||
| export type ParagraphStyle = { | ||
| export type ParagraphStyle = Required<BaseTextStyle> & { |
There was a problem hiding this comment.
You lack fontKey as being required but nullable >> https://github.com/chili-publish/studio-engine/blob/main/packages/document_logic_engine/lib/src/stylekit/model/paragraph_style/paragraph_style_dto.generating.dart#L21
| trackingRight?: string; | ||
| textIndent?: string; | ||
| // end of unit properties | ||
| fillColorApplied?: boolean; |
There was a problem hiding this comment.
I know we discussed this in a call and I realize this is there because of my limited understanding of types in typescript.
All the optional properties as of now in textStyles types on SDK are actually required and sometimes nullable types in dart.
So you may safely remove the three fillColorApplied, underline & lineThrough from this type (as inheriting them from BaseTextStyle would make it consistent).
Now all the BaseTextStyle properties types can be made required (& nullable if you chose it to be your default) to match dart.
Note: We're not entirely consistent for all the other types in the SDK either, (not a huge issue for an integrator but we should probably match more type to the ones from the engine, this is mostly a message to me and our team).
| @@ -1,7 +1,7 @@ | |||
| import { EditorAPI, Id } from '../types/CommonTypes'; | |||
| import { getEditorResponseData } from '../utils/EditorResponseData'; | |||
| import { ImageSourceTypeEnum, ImageFrameVariableSource } from '../types/FrameTypes'; | |||
| import { TextType } from '../types/TextTypes'; | |||
|
This PR should be merged into long-lived v2 branch, to fix the issue here we could create a non breaking union type |
This PR aligns all text-style related types to better match the use-cases of workspace.
Related tickets
Screenshots