aminghadersohi commented on PR #43567: URL: https://github.com/apache/superset/pull/43567#issuecomment-6324570760
@gkneighb @rusackas thanks both. Taking the judgement calls on my threads: - Row-limit/color-scheme test fixture: not required. If you add it, great, but it doesn't block. - `_funnel_chart_what` duplicating `_pie_chart_what`: leave it. It's pre-existing and shared with pie, so a consolidation belongs in its own PR. - `sort_by_metric` resetting on update: leave it for the same reason. It's pre-existing and shared with pie, so it's a follow-up and not part of this PR. I haven't reviewed the changes made after my last review: `exclude_unset=True` in `funnel.py`, the funnel Vega-Lite preview moved into the plugin's `vega_lite_preview` hook, and the emptied `_FALLBACK_VEGA_RENDERERS`. Once the conflict with master is resolved, I'll re-review the new head, including that the `sort_by_metric` → `orderby` fix still fires, and then this can merge. -- 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]
