durgaprasadml commented on PR #41536: URL: https://github.com/apache/superset/pull/41536#issuecomment-5072836483
> Thanks for updating the title/description to match the actual fix, @durgaprasadml. The `ResizeObserver` approach in `Chart.tsx` makes sense. > > Two things before this can merge, though. The description says regression tests were added, but the diff only touches `Chart.tsx`, no test file at all. Can you actually add the coverage you described (expanded-on-load, height updates after async markdown render)? > > Also, the `lodash` to `lodash-es` swap for `debounce` looks unrelated to the fix. It's not used anywhere else in the codebase and isn't a direct dependency, just a transitive one, so it seems risky to lean on it here. Mind reverting that part or explaining further? Thanks for the detailed review! I've addressed both of the remaining points: 1. **Regression tests:** I've added the missing test coverage for the ResizeObserver-based fix. The new tests cover the expanded-on-load scenario and verify that the description height is updated correctly after the asynchronous markdown content has been rendered. 2. **`lodash-es` import:** I've reverted the `lodash-es` change. The fix doesn't depend on it, so I've kept the existing `lodash/debounce` import to avoid introducing an unrelated dependency change. Please let me know if there's anything else you'd like me to adjust. Thanks again for the review! -- 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]
