yardz commented on pull request #13422: URL: https://github.com/apache/superset/pull/13422#issuecomment-794496303
> Personally, when I think of a table I think of it in its entirety, rarely am I thinking about the header and body as 2 completely separate entities, and a header row separately from a header I agree with that, when using a table we really think of all the set. When using a table component, it doesn't really make sense to call the table, header, body separately. We need to send the header and the data and the "magic" must happen. But in this case it is the construction of the table and not the use of a table (other components will use that table). So, in my view, this division makes sense. It is also worth mentioning that this component has a behavior logic (like props loading) and some functions to add different props on html entities (like getTableProps and getTableBodyProps). ### Tests The goal here was to create tests. The end result was 7 test files with an average of 100 test lines in each and not all behavior scenarios are covered (only the most important ones). If we put all the tests into a single component, we would have an archive of approximately 700 test lines, something that is not easy to maintain in long term. In general, I create tests and components with TDD, so large and complex components need to be broken to be workable. ### Complexity About complexity. The final components are less complex. When I refer to complexity I mean cyclomatic complexity. Code with less [cyclomatic complexity](https://en.wikipedia.org/wiki/Cyclomatic_complexity) is easier to understand because it is simpler. A person who is unaware of react-tables (like me - I’ve never seen this before) and is not extremely familiar with the old component you will not be able to understand. I spent a few hours myself to understand what was being done there... And I only started to move forward when I started to delete code to see what happened. It was not an easy task at all. ### Clean Code Of course, you know. But [this link](https://gist.github.com/wojteklu/73c6914cc446146b8b533c0988cf8d29) has a short list of general topics that are an excellent guide for day to day. Basically one of the general rules is `Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.` (personally is my favorite). ### useMemo I saw no need, but since the original component was memoized `export default React.memo` I preferred to keep something in the same way. Anyway, I don't think that useMemo makes reading difficult to deserve attention. @nytai If even with these arguments you are not convinced I can close the PR and return with the old code, and do the tests that I can. ---------------------------------------------------------------- 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]
