michael-s-molina commented on pull request #13497: URL: https://github.com/apache/superset/pull/13497#issuecomment-793813899
Hey @yardz this is a big one! Since you broke the component into many parts I'll try to break the review also 😆. I'll focus more on general topics instead of code lines: - You did a great job with typescript support. All files are in typescript and the types are well defined. - The tests also look great and this will definitely increase test coverage. - I do have a concern about introducing regressions because of the changes. We definitely need to execute acceptance tests here. The next part is subjective, there is no right or wrong, so feel free to apply any recommendation: - I think we can slightly increase the limit for breaking a component. Some components are very simple, non-reusable, and appear to be an internal part of the parent component. We don't want the original size of the component but also don't want to end up with labels or simple divs as separate components. A lot of fragmentation also doesn't help with readability. The key here is **balance**. - Hooks are essentially **reusable** functions. You generally create them when you want to share stateful logic between components. Some of the hooks that you created are not reusable but internal functions of this component. I think they belong to the subcomponents that you created. As I said these are subjective, so feel free to apply any 😉 ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
