-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat(layers): support per-object text max width #9747
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: master
Are you sure you want to change the base?
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.
Pull Request Overview
This PR adds support for per-object text max width in TextLayer by introducing a getMaxWidth
accessor function. This enhancement allows individual text labels to have their own wrapping width limits, providing more granular control over text layout.
Key changes:
- Added
getMaxWidth
accessor to TextLayer for per-object width control - Updated text transformation logic to use object-specific max width values
- Exposed the new accessor through GeoJsonLayer for broader usability
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
modules/layers/src/text-layer/text-layer.ts |
Implements the core getMaxWidth accessor functionality and updates text transformation logic |
modules/layers/src/geojson-layer/sub-layer-map.ts |
Maps getTextMaxWidth prop to TextLayer's getMaxWidth accessor |
docs/api-reference/layers/text-layer.md |
Documents the new getMaxWidth accessor with usage examples |
docs/api-reference/layers/geojson-layer.md |
Documents the getTextMaxWidth prop forwarding to TextLayer |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
objectMaxWidth = value; | ||
} | ||
} else if (Number.isFinite(maxWidthAccessor) && maxWidthAccessor >= 0) { | ||
objectMaxWidth = maxWidthAccessor; |
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.
[nitpick] The variable maxWidthAccessor
is unnecessary since getMaxWidth
is already descriptive. Consider using getMaxWidth
directly in the conditional checks below to reduce code complexity.
objectMaxWidth = maxWidthAccessor; | |
if (typeof getMaxWidth === 'function') { | |
const value = getMaxWidth(object, objectInfo); | |
if (Number.isFinite(value) && value >= 0) { | |
objectMaxWidth = value; | |
} | |
} else if (Number.isFinite(getMaxWidth) && getMaxWidth >= 0) { | |
objectMaxWidth = getMaxWidth; |
Copilot uses AI. Check for mistakes.
objectMaxWidth = value; | ||
} | ||
} else if (Number.isFinite(maxWidthAccessor) && maxWidthAccessor >= 0) { | ||
objectMaxWidth = maxWidthAccessor; |
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.
[nitpick] This condition handles the case where getMaxWidth
is a number rather than a function, but this seems inconsistent with the accessor pattern used elsewhere in the codebase. Consider if this number fallback is necessary or if it should always be treated as an accessor function.
objectMaxWidth = maxWidthAccessor; |
Copilot uses AI. Check for mistakes.
Summary
getMaxWidth
accessor to TextLayergetMaxWidth
and expose through GeoJsonLayerThe layout is handled entirely on the CPU. TextLayer.transformParagraph resolves the effective maxWidth for each datum and runs the transformParagraph utility to wrap/truncate the text before any geometry is sent to the GPU, so the shader simply receives per-character offsets that already reflect the width limit. The sublayer that actually renders each glyph (MultiIconLayer) just reuses the existing IconLayer shaders and adds SDF uniforms; it does not read or use maxWidth, so no changes to shader code are necessary.
Questions
Testing
yarn test node test/modules/layers/text-layer/text-layer.spec.ts
(fails: SyntaxError Unexpected identifier 'assert')https://chatgpt.com/codex/tasks/task_i_68a736833ef0832c99608800972619f1