-
Notifications
You must be signed in to change notification settings - Fork 53
refactor: reorganize style resolution in the layout engine (SD-1411) #1786
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: main
Are you sure you want to change the base?
refactor: reorganize style resolution in the layout engine (SD-1411) #1786
Conversation
9ac7d31 to
68820c0
Compare
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f93d3c2b3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const markerRunProperties = resolveRunProperties( | ||
| converterContext!, | ||
| resolvedParagraphProperties.runProperties, | ||
| resolvedParagraphProperties, | ||
| converterContext!.tableStyleId, |
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.
Guard optional converterContext before list marker resolution
Here converterContext is asserted non-null to resolve list marker run properties, but computeParagraphAttrs accepts an optional converterContext and the adapter entrypoints allow it to be omitted. If a caller runs toFlowBlocks/paragraph conversion without a converterContext (valid per types) on a document that still has numberingProperties + listRendering, this path will dereference converterContext!.tableStyleId and throw instead of falling back to defaults, which is a regression from the previous optional behavior.
Useful? React with 👍 / 👎.
| const normalizedAlignment = normalizeAlignment(resolvedParagraphProperties.justification); | ||
| const normalizedBorders = normalizeParagraphBorders(resolvedParagraphProperties.borders); | ||
| const normalizedShading = normalizeParagraphShading(resolvedParagraphProperties.shading); | ||
| const paragraphDecimalSeparator = DEFAULT_DECIMAL_SEPARATOR; | ||
| const tabIntervalTwips = DEFAULT_TAB_INTERVAL_TWIPS; |
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.
Preserve locale decimal separator for decimal tab alignment
Decimal tab alignment depends on the paragraph decimal separator that previously came from styleContext defaults or document locale; this is now hard-coded to '.' for every paragraph. For documents whose default decimal separator is ',' (or any non-dot locale), decimal tab stops will align on the wrong character and produce visibly incorrect tabbed number layouts. The previous logic pulled the locale-specific separator, so this change drops that configuration path.
Useful? React with 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.