chore: Moves Fade component into TableElement

[michael-s-molina]

SUMMARY

Moves Fade component into TableElement. It didn't make the cut to be a component in itself 😆.

TEST PLAN

1 - Execute TableElement tests.
2 - All tests should pass.

@rusackas @junlincc

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

Merging #13458 (159a596) into master (6026e7d) will increase coverage by 0.73%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #13458      +/-   ##
==========================================
+ Coverage   72.74%   73.48%   +0.73%     
==========================================
  Files         906      604     -302     
  Lines       45654    21249   -24405     
  Branches     5516     5515       -1     
==========================================
- Hits        33212    15614   -17598     
+ Misses      12232     5508    -6724     
+ Partials      210      127      -83     

Flag	Coverage Δ
cypress	57.96% <33.33%> (?)
hive	?
javascript	63.39% <100.00%> (-0.01%)	⬇️
mysql	?
postgres	?
presto	?
python	?
sqlite	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...et-frontend/src/SqlLab/components/TableElement.jsx	88.76% <100.00%> (+0.39%)	⬆️
superset-frontend/src/views/routes.tsx	56.81% <0.00%> (-6.82%)	⬇️
...ore/components/DatasourcePanel/DatasourcePanel.tsx	75.86% <0.00%> (-2.71%)	⬇️
...-frontend/src/datasource/ChangeDatasourceModal.tsx	84.33% <0.00%> (-1.03%)	⬇️
...t-frontend/src/views/CRUD/welcome/SavedQueries.tsx	62.50% <0.00%> (-0.66%)	⬇️
...erset-frontend/src/SqlLab/components/ResultSet.tsx	68.22% <0.00%> (-0.30%)	⬇️
.../src/components/dataViewCommon/TableCollection.tsx	100.00% <0.00%> (ø)
superset/datasets/api.py
superset/views/dashboard/mixin.py
superset/tasks/scheduler.py
... and 501 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6026e7d...10de2b3. Read the comment docs.

[junlincc]

@yardz @geido let's review. 🙏

[geido]

I have mixed feelings about this. I know the component is negligible in size and purpose but I think I like the fact that you can use it as a small utility component. Either way, the code LGTM and it can go if it reaches consensus.

[michael-s-molina]

I have mixed feelings about this. I know the component is negligible in size and purpose but I think I like the fact that you can use it as a small utility component. Either way, the code LGTM and it can go if it reaches consensus.

@geido I would say that when you look at the component's name, you can imagine a reusable component used in multiple places. But when you look at the implementation and props you will see that is just a div with almost no behavior and used only in TableElement. I have difficulty imagining creating tests and a storybook for this at its current stage. Maybe in the future, if a Fade component is required in other places and with more behavior, we can move it to src/components.

[yardz]

@geido I would say that when you look at the component's name, you can imagine a reusable component used in multiple places. But when you look at the implementation and props you will see that is just a div with almost no behavior and used only in TableElement. I have difficulty imagining creating tests and a storybook for this at its current stage. Maybe in the future, if a Fade component is required in other places and with more behavior, we can move it to src/components.

I have a very strong opinion about not assembling components (to be honest i will always be against it).
I did a very quick and superficial search and saw several components using "some kind" of fade... Which brings me to the question, why this fade is not being used in more places?
I believe that a piece of the reason is precisely because there is no fade in src/components and the idea that "if a component is used only here, I will put it here".

But I don't see anything wrong with the code. I just disagree with the PR's goal.

[michael-s-molina]

I have a very strong opinion about not assembling components (to be honest i will always be against it).

For me, always is a very strong word in the context of software development. I tend to analyze case by case considering the context.

I did a very quick and superficial search and saw several components using "some kind" of fade... Which brings me to the question, why this fade is not being used in more places?

Thank you for the search. That's a strong indicator that in fact, we should have a Fade component in src/components. Can you paste the references here so we can close this PR and open a new one with the objective of creating this new Fade component and consolidate all the references?

I believe that a piece of the reason is precisely because there is no fade in src/components and the idea that "if a component is used only here, I will put it here".

I understand that if we already have a component available that will prevent duplications, but this does not mean that every little bit of code that we do should be in the components folder. Generally, when we have classical components like Button we start there, but a lot of components evolve to a shared component and end up in the components folder by merit. I think that this is the case with Fade. If we identify multiple places with this behavior and resolve all use cases using a nice API, then we can move it there.

[yardz]

Thank you for the search. That's a strong indicator that in fact, we should have a Fade component in src/components. Can you paste the references here so we can close this PR and open a new one with the objective of creating this new Fade component and consolidating all the references?

These components have some "fade behavior":

• src/dashboard/components/gridComponents/ChartHolder.jsx
• src/chart/Chart.jsx
• src/messageToasts/components/ToastPresenter.tsx

There are also "fade effects" in less files that I was unable to find the components that use them. Again, this search was very superficial (I searched for the name "fade" And I did a very simple reading of the code). If we look for "transition" + "opacity" effects, I believe we will find more.

[yardz]

For me, always is a very strong word in the context of software development. I tend to analyze case by case considering the context.

I agree 100%, that's why I said I have a very strong opinion in this case hahaha
In this case I really think it’s always

But again, this is my opinion and not a pattern of this project, you are totally free to disagree and this is not a block for the merge.

[rusackas]

/testenv up

[github-actions]

@rusackas Ephemeral environment spinning up at http://18.236.206.238:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[rusackas]

I think the way this fade component is used, with all of the state management and so forth, is probably more complex than it needs to be. We can circle back later, but I'm willing to bet a refactor of this could greatly simplify things.

TL;DR: LGTM.

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.