giuliosmall commented on PR #35033: URL: https://github.com/apache/superset/pull/35033#issuecomment-3261573663
> Thanks for addressing this important visualization bug. Before merging, please consider: > > Package-lock.json: Please revert the unintentional changes to package-lock.json > > Fundamental approach: Rather than ensuring min/max are included after rounding, consider using Math.floor(minValue) and Math.ceil(maxValue) for the boundary breakpoints. This would: > > * Guarantee all data falls within ranges > * Provide cleaner legend labels > * Eliminate the complex adjustment logic > > Edge case testing: address the automated review concern about minimum value exclusion in a specific unit test. > > The precision calculation logic is sound, but the directional rounding approach might be more robust than the current fix. Thanks @mistercrunch, your suggestion sounds solid. - Proceeded to rollback package-lock.json - Applied Math.floor to simplify the logic in. [7d3d165](https://github.com/apache/superset/pull/35033/commits/7d3d165ef99c74938dca2d8d354d821d5d1862e7). Tests are passing: ```bash npm test -- plugins/legacy-preset-chart-deckgl/src/utils.test.ts > [email protected] test > cross-env NODE_ENV=test NODE_OPTIONS="--max-old-space-size=8192" jest --max-workers=80% --silent plugins/legacy-preset-chart-deckgl/src/utils.test.ts jest-haste-map: duplicate manual mock found: mockExportObject The following files share their name; please delete one of them: * <rootDir>/spec/__mocks__/mockExportObject.js * <rootDir>/packages/superset-ui-core/__mocks__/mockExportObject.js jest-haste-map: duplicate manual mock found: mockExportString The following files share their name; please delete one of them: * <rootDir>/spec/__mocks__/mockExportString.js * <rootDir>/packages/superset-ui-core/__mocks__/mockExportString.js jest-haste-map: duplicate manual mock found: svgrMock The following files share their name; please delete one of them: * <rootDir>/spec/__mocks__/svgrMock.tsx * <rootDir>/packages/superset-ui-core/__mocks__/svgrMock.tsx PASS plugins/legacy-preset-chart-deckgl/src/utils.test.ts Test Suites: 1 passed, 1 total Tests: 24 passed, 24 total Snapshots: 0 total Time: 3.923 s, estimated 4 s ``` Please, take another final look and let me know if it's ready to be merged as this is my first time committing to Superset. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
