Feat/add more kepler map export resultions resolutions - #3351
Conversation
Signed-off-by: bdjulbic <bdjulbic@foursquare.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: bdjulbic <bdjulbic@foursquare.com>
Signed-off-by: bdjulbic <bdjulbic@foursquare.com>
There was a problem hiding this comment.
Pull request overview
This PR appears intended to expand Kepler.gl map image export resolution options, touching the export sizing logic, UI state/types, the export modal UI, and associated tests/docs.
Changes:
- Reworked
RESOLUTIONSfrom{ONE_X, TWO_X}to a set of fixed-size presets (e.g.SIZE_1024_768,SIZE_1920_1080). - Removed ratio/resolution configuration from the export image modal and from the
ExportImagetype/defaults. - Simplified/removed several tests and updated docs around
DEFAULT_EXPORT_IMAGE.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/node/utils/export-utils-test.js | Updates calculateExportImageSize tests to expect a 0×0 result. |
| test/node/reducers/ui-state-test.js | Changes SET_EXPORT_IMAGE_SETTING test to assert legend only. |
| test/browser/components/modals/export-image-modal-test.js | Removes assertions for ratio/resolution option rendering/interaction. |
| src/utils/src/export-utils.ts | Replaces calculateExportImageSize implementation with a constant 0×0 return. |
| src/types/reducers.d.ts | Removes ratio/resolution from the ExportImage type. |
| src/reducers/src/ui-state-updaters.ts | Removes ratio/resolution defaults from DEFAULT_EXPORT_IMAGE. |
| src/constants/src/default-settings.ts | Adds multiple fixed-size resolution presets and updates resolution option definitions. |
| src/components/src/plot-container.tsx | Adds legend scaling remap and applies scaling via CSS zoom + CSS variable. |
| src/components/src/modals/export-image-modal.tsx | Removes ratio/resolution selection UI; keeps legend toggle + preview. |
| docs/api-reference/reducers/ui-state.md | Updates documented DEFAULT_EXPORT_IMAGE properties/defaults. |
Comments suppressed due to low confidence (2)
src/components/src/modals/export-image-modal.tsx:91
- The export image modal no longer provides any controls for selecting export size/resolution (or ratio), even though the PR adds new resolution constants/options. As a result, users can’t choose the newly added resolutions, and
exportImagesettings needed for sizing may never be set. Please reintroduce/update the UI to select among the new resolution options (and/or ratio), or remove the unused resolution work if the feature is being deprecated.
return (
<StyledModalContent className="export-image-modal">
<ImageOptionList>
<div className="image-option-section">
<div className="image-option-section-title">
<FormattedMessage id={'modal.exportImage.mapLegendTitle'} />
</div>
<Switch
type="checkbox"
id="add-map-legend"
checked={legend}
label={intl.formatMessage({id: 'modal.exportImage.mapLegendAdd'})}
onChange={() => onUpdateImageSetting({legend: !legend})}
/>
</div>
</ImageOptionList>
<ImagePreview exportImage={exportImage} />
src/reducers/src/ui-state-updaters.ts:142
DEFAULT_EXPORT_IMAGEno longer sets defaults forratio/resolution, but other parts of the export flow still depend on these settings to computeimageSize(and TS code still expects them onexportImage). This makes the default export configuration incomplete and contributes to 0×0 exports. Please restore coherent defaults (e.g. default ratio + a default fixed-size resolution) or update the rest of the export pipeline to not depend on these fields.
export const DEFAULT_EXPORT_IMAGE: ExportImage = {
// user options
legend: false,
mapH: 0,
mapW: 0,
imageSize: {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| test('#uiStateReducer -> SET_EXPORT_IMAGE_SETTING', t => { | ||
| const newReducer = reducer( | ||
| INITIAL_UI_STATE, | ||
| setExportImageSetting({resolution: RESOLUTIONS.TWO_X}) | ||
| setExportImageSetting({legend: true}) | ||
| ); | ||
|
|
||
| const expectedState = { | ||
| ...INITIAL_UI_STATE, | ||
| exportImage: { | ||
| ...INITIAL_UI_STATE.exportImage, | ||
| resolution: RESOLUTIONS.TWO_X | ||
| legend: true | ||
| } | ||
| }; | ||
|
|
||
| t.deepEqual(newReducer, expectedState, 'should set the resolution to TWO_X'); | ||
| t.deepEqual(newReducer, expectedState, 'should set the legend to true'); | ||
|
|
There was a problem hiding this comment.
The reducer test for SET_EXPORT_IMAGE_SETTING was changed to only cover legend. If resolution/ratio are still supported settings (and especially with the new resolution list), this test suite should also cover updating those fields and the resulting imageSize calculation behavior to prevent regressions.
| test('exportUtils -> calculateExportImageSize', t => { | ||
| t.deepEqual( | ||
| calculateExportImageSize({ | ||
| mapW: 1400, | ||
| mapH: 990, | ||
| ratio: EXPORT_IMG_RATIOS.SCREEN, | ||
| resolution: RESOLUTIONS.ONE_X | ||
| }), | ||
| {scale: 1, imageW: 1400, imageH: 990}, | ||
| calculateExportImageSize({}), | ||
| {scale: 1, imageW: 0, imageH: 0}, | ||
| 'Should calculate the correct export image size' | ||
| ); |
There was a problem hiding this comment.
This test now only asserts that calculateExportImageSize({}) returns a 0×0 size, which won’t catch regressions in the actual export sizing logic (and effectively codifies the current broken behavior). Please add back coverage for computing sizes from real inputs (mapW/mapH + the intended ratio/resolution settings), including at least one of the newly added fixed-size resolutions.
|
|
||
| import {EXPORT_IMG_RATIO_OPTIONS, EXPORT_IMG_RESOLUTION_OPTIONS} from '@kepler.gl/constants'; | ||
| import {ExportImage} from '@kepler.gl/types'; | ||
| import {StyledModalContent, SelectionButton, CheckMark} from '../common/styled-components'; |
There was a problem hiding this comment.
SelectionButton and CheckMark are imported but no longer used after removing the ratio/resolution option UI. Please remove these unused imports to avoid lint/no-unused-vars failures and reduce bundle noise.
| import {StyledModalContent, SelectionButton, CheckMark} from '../common/styled-components'; | |
| import {StyledModalContent} from '../common/styled-components'; |
|
|
||
| /* Apply logarithmic zoom to legend panel */ | ||
| .map-control-panel { | ||
| zoom: var(--legend-scale, 1) !important; |
There was a problem hiding this comment.
Using the CSS zoom property for .map-control-panel is non-standard and not supported consistently across browsers (notably Firefox). For more reliable export rendering, prefer a standards-based approach like transform: scale(...) with an appropriate transform-origin, or apply scaling via layout calculations instead of zoom.
| zoom: var(--legend-scale, 1) !important; | |
| transform-origin: top left; | |
| transform: scale(var(--legend-scale, 1)) !important; |
|
|
||
| - `ratio` **[string][57]** Default: `'SCREEN'`, | ||
| - `resolution` **[string][57]** Default: `'ONE_X'`, | ||
| - `resolution` **[string][57]** Default: `'SIZE_1024_768'`, |
There was a problem hiding this comment.
The docs now state DEFAULT_EXPORT_IMAGE has a resolution default of 'SIZE_1024_768', but the reducer default (DEFAULT_EXPORT_IMAGE) and the exported ExportImage type in this PR no longer include a resolution field. Please align the documentation with the actual state shape (either document the new model, or restore the resolution field in code/types).
| - `resolution` **[string][57]** Default: `'SIZE_1024_768'`, |
| @@ -40,59 +40,6 @@ test('Components -> ExportImageModal.mount', t => { | |||
|
|
|||
| t.equal(wrapper.find(ImagePreview).length, 1, 'should render ImagePreview'); | |||
|
|
|||
There was a problem hiding this comment.
This mount test no longer verifies any of the export sizing controls (ratio/resolution) in the modal. Given the PR goal of adding more export resolutions, please add assertions that the modal renders the new resolution options and that selecting one calls onUpdateImageSetting with the expected payload (or, if the UI is intentionally removed, update tests to cover the new intended mechanism for setting export dimensions).
| const selectionButtons = wrapper.find(SelectionButton); | |
| t.ok(selectionButtons.length > 0, 'should render export sizing SelectionButton options'); | |
| const previousCallCount = onUpdateImageSetting.callCount; | |
| selectionButtons.at(0).simulate('click'); | |
| t.ok( | |
| onUpdateImageSetting.callCount > previousCallCount, | |
| 'should call onUpdateImageSetting when selecting an export resolution option' | |
| ); | |
| const lastCallArgs = onUpdateImageSetting.lastCall.args[0]; | |
| t.equal( | |
| typeof lastCallArgs, | |
| 'object', | |
| 'onUpdateImageSetting should be called with a settings object when selecting export resolution' | |
| ); |
| export function calculateExportImageSize({}: {}) { | ||
| return { | ||
| scale, | ||
| imageW, | ||
| imageH | ||
| scale: 1, | ||
| imageW: 0, | ||
| imageH: 0 | ||
| }; |
There was a problem hiding this comment.
calculateExportImageSize currently ignores all inputs and always returns {scale: 1, imageW: 0, imageH: 0}. This will propagate a 0×0 export size into uiState.exportImage.imageSize, and downstream the export pipeline passes width/height: 0 into dom-to-image, likely producing blank/invalid exports. Please restore a real implementation that derives imageW/imageH (and scale, and ideally preserves/returns zoomOffset) from the current export settings (e.g., mapW/mapH plus ratio/resolution options), and return null when map dimensions are invalid so callers can fall back to the previous imageSize.
| */ | ||
| function remapLegendScale(scale: number): number { | ||
| const max = 5; | ||
| const t = (scale - 1) / (max - 1); |
There was a problem hiding this comment.
remapLegendScale claims to remap export scale from [1..5] into [1..3], but it doesn’t clamp the input or output. If scale exceeds 5 (which is possible when exporting large fixed sizes from a small viewport), the legend scale will exceed 3 and the comment’s guarantee won’t hold. Consider clamping scale (or t) to the expected range before remapping.
| const t = (scale - 1) / (max - 1); | |
| // Clamp scale to the expected input range to ensure the output stays within [1..3] | |
| const clampedScale = Math.min(Math.max(scale, 1), max); | |
| const t = (clampedScale - 1) / (max - 1); |
No description provided.