rusackas commented on PR #33247: URL: https://github.com/apache/superset/pull/33247#issuecomment-4713430129
@aminghadersohi great catch on the `null` case, that one would have quietly zeroed out legacy charts. Fixed in 7e7f38df92 by coercing through the fallback first (`Number(value ?? fallback)`), so `null`/`undefined` land on the default while an explicit `0` is preserved, plus a test for it. Also hoisted `toNumber` to module scope. On the units asymmetry: that split is deliberate, same reasoning as the backward-compat thread above. Old charts have no `point_radius_units` so both paths should resolve to `meters`... the dashboard path does; the explore default pulling `pixels` is the one inconsistency. I went back and forth and would rather new charts default to pixels than make legacy charts in explore drift, but I take your point that it is two outputs from one saved chart. Happy to align the control default to `meters` if you feel strongly. Left the `validateInteger` vs `legacyValidateInteger` nit alone for now, since modernizing the two neighboring controls feels like its own change. -- 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]
