Feat continuous legend handler text style function#2052
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for computing slider/continuous-legend handler text styles dynamically via a callback, enabling per-value/per-handle styling at render and update time.
Changes:
- Extends
handlerText.styleto accept a callback with(value, position, context)for dynamic text styling. - Refactors slider handler-text rendering/updating to reuse a shared attribute builder and invoke the callback when provided.
- Adds unit tests for the new callback behavior in
Sliderand continuous legends.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/vrender-components/src/slider/type.ts | Introduces handler text style callback/context types; reuses layout/align type aliases. |
| packages/vrender-components/src/slider/slider.ts | Implements callback evaluation and refactors handler text attribute computation for render/update paths. |
| packages/vrender-components/tests/unit/slider.test.ts | Adds a unit test covering style callback invocation on render and setValue. |
| packages/vrender-components/tests/unit/legend/continuous.test.ts | Adds unit tests ensuring continuous legends evaluate the style callback on render and setSelected. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(style).toHaveBeenLastCalledWith( | ||
| 78, | ||
| 'start', | ||
| expect.objectContaining({ | ||
| layout: 'horizontal', | ||
| align: 'bottom', | ||
| railWidth: 200, | ||
| railHeight: 10 | ||
| }) | ||
| ); | ||
| expect(startHandlerText.attribute.fill).toBe('red'); | ||
| expect(startHandlerText.attribute.dx).toBe(-6); | ||
| }); |
There was a problem hiding this comment.
This new test asserts that the style callback is called with position: 'start' after slider.setValue(78), but the initial render in this same test expects position: 'end'. For a single-handler slider, the callback position should be consistent across render and updates; once the implementation is fixed, update the expectation (and the asserted dx) to match the intended consistent position.
| } from '@visactor/vrender-core'; | ||
|
|
||
| type Text = string | number; | ||
| type SliderLayout = 'horizontal' | 'vertical' | string; |
There was a problem hiding this comment.
type SliderLayout = 'horizontal' | 'vertical' | string collapses to just string, so consumers lose the literal union benefits (autocomplete/narrowing) while still implying there are special cases. If the goal is “allow any string but prefer these literals”, consider using a literal-union pattern (e.g. 'horizontal' | 'vertical' | (string & {})) or remove | string if only these two are supported.
| type SliderLayout = 'horizontal' | 'vertical' | string; | |
| type SliderLayout = 'horizontal' | 'vertical' | (string & {}); |
| const updateHandlerText = | ||
| handler.name === SLIDER_ELEMENT_NAME.startHandler ? this._startHandlerText : this._endHandlerText; | ||
| if (updateHandlerText) { | ||
| const { handlerText = {} } = this.attribute as SliderAttributes; | ||
| updateHandlerText.setAttributes({ | ||
| text: handlerText.formatter ? handlerText.formatter(value) : value.toFixed(handlerText.precision ?? 0), | ||
| [isHorizontal ? 'x' : 'y']: position | ||
| }); | ||
| const handlerPosition = handler.name === SLIDER_ELEMENT_NAME.startHandler ? 'start' : 'end'; | ||
| updateHandlerText.setAttributes(this._getHandlerTextAttributes(value, handlerPosition)); | ||
| } |
There was a problem hiding this comment.
In single-handler mode (range falsy), _renderHandlers calls _renderHandlerText(startValue, 'end'), but _updateHandler derives handlerPosition from the handler name and passes 'start' for startHandler. That makes the handlerText.style callback receive different position values between initial render and subsequent updates. Consider deriving the callback position based on range (e.g., treat startHandler as 'end' when range is false) so render/update are consistent.
| [isHorizontal ? 'x' : 'y']: position, | ||
| text: handlerTextAttr.formatter ? handlerTextAttr.formatter(value) : value.toFixed(handlerTextAttr.precision ?? 0) | ||
| }); | ||
| const handlerPosition = handlerText.name === SLIDER_ELEMENT_NAME.startHandlerText ? 'start' : 'end'; |
There was a problem hiding this comment.
_updateHandlerText infers the callback position solely from handlerText.name (start => 'start', end => 'end'). In single-handler mode the only text element is startHandlerText but it is rendered with position 'end' (_renderHandlers(... range ? 'start' : 'end')). This causes handlerText.style callbacks to see inconsistent positions between render and drag/update paths. Suggest aligning this logic with _renderHandlers by also checking this.attribute.range when determining 'start' | 'end'.
| const handlerPosition = handlerText.name === SLIDER_ELEMENT_NAME.startHandlerText ? 'start' : 'end'; | |
| // 对齐 _renderHandlers 中对 position 的推断： | |
| // range 模式下根据名称区分 start/end；非 range（单滑块）模式下统一视为 'end' | |
| const handlerPosition = | |
| this.attribute.range && handlerText.name === SLIDER_ELEMENT_NAME.startHandlerText ? 'start' : 'end'; |
xuefei1313
deleted the
feat-continuous-legend-handler-text-style-function
branch
[中文版模板 / Chinese template]
🤔 This is a ...
🔗 Related issue link
🐞 Bugserver case id
💡 Background and solution
📝 Changelog
☑️ Self-Check before Merge
🚀 Summary
copilot:summary
🔍 Walkthrough
copilot:walkthrough