durgaprasadml commented on PR #41536:
URL: https://github.com/apache/superset/pull/41536#issuecomment-5220093212
> @durgaprasadml this has conflicts with master now, needs a rebase before
it can merge.
>
> Also still waiting on the lodash bit from my last comment: `Chart.tsx`
imports `debounce` from `lodash/debounce` right now, but master's been on
`import { debounce } from 'lodash-es'` since #41511's tree-shaking migration.
Can you flip that back?
>
> One more small thing while you're in there: the new test block wraps
everything in `describe('Chart Description with ResizeObserver', ...)`, but
every other test in this file is a flat `test(...)`. Mind un-nesting those so
it matches the rest of the file?
>
> Let me know if you'd like me to tackle any of this (assuming I can push to
your fork).
Thanks for the detailed review and for catching those!
I've addressed everything you mentioned:
- Rebased the branch on the latest `master` to resolve the merge conflicts.
- Switched the `debounce` import back to `import { debounce } from
'lodash-es'` to match the tree-shaking migration introduced in #41511.
- Removed the nested `describe(...)` block and converted the new tests to
standalone `test(...)` cases so they match the existing style in
`Chart.test.tsx`.
I've pushed the updated changes. When you have a chance, could you please
take another look?
Thanks again for the thorough review and for offering to help!
--
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]
