-
Notifications
You must be signed in to change notification settings - Fork 29
Use TextLayout instead of TextType #120
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
Conversation
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.
Looks good, let us know when it's deployed to prod and we should run the job to deploy.
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.
Lgtm but with a caveat that we should consider when making future updates to the changelog.
@@ -224,7 +230,7 @@ You must provide trader details by February 16, 2025, to keep your add-on visibl | |||
- Added many new **Text APIs** for improved text management. | |||
- [`TextNode.fullContent`](./document-sandbox/document-apis/classes/TextNode.md#fullcontent) accessor: returns the [`TextContentModel`](./document-sandbox/document-apis/classes/TextContentModel.md) containing the complete text string and its styles associated to the Text Flow (Threaded Text or Overflow Text). | |||
- [`TextNode.nextTextNode`](./document-sandbox/document-apis/classes/TextNode.md#nexttextnode) accessor: gets the next node that overflowing text will spill into. | |||
- [`TextNode.layout`](./document-sandbox/document-apis/classes/TextNode.md#layout) accessor: gets and sets the [`TextType`](./document-sandbox/document-apis/enumerations/TextType.md) of the text node frame. | |||
- [`TextNode.layout`](./document-sandbox/document-apis/classes/TextNode.md#layout) accessor: gets and sets the [`TextLayout`](./document-sandbox/document-apis/enumerations/TextLayout.md) of the text node frame. |
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.
Nit: this particular change seems ok but we should be careful about updating descriptions of older API changes to reflect the latest state of the API. That may be confusing since this description is now mixing old and new and may not accurately describe any state the API has ever existed in.
In some cases it might even be impossible to keep the links in older changelog entries up to date, e.g. if an API has changed a lot or was removed. We should probably decide a policy for how much to update these entries and what to do if it's impossible to keep links working (would we rather leave the older link and let is 404, or strip the link out of the text, or add a new parenthetical note that links to the newer equivalent API, etc.).
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.
You've hit a nerve, @peterflynn! I always struggle with links in the changelog. @nimithajalal suggested archiving the oldest entries, which I'm all in for.
IIRC, the linter is part of the build script, and it'd prevent the merge if it found something wrong there—so we're in this weird situation where we need to fix links even if they've stopped making sense.
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.
LGTM
Description
Renamed TextType enum to TextLayout
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: