Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove old js components: moving some logic to typescript #6550

Merged
merged 22 commits into from
Aug 4, 2023
Merged

Remove old js components: moving some logic to typescript #6550

merged 22 commits into from
Aug 4, 2023

Conversation

antgamdia
Copy link
Contributor

@antgamdia antgamdia commented Aug 1, 2023 •
edited
Loading

Description of the change

This PR is following up #6539. In this one, the focus is on:

  • Transforming Row and Column to tsx components.
  • Update some imports
  • Transform the useOutsideClick hook to typescript.
  • Remove an unnecessary isSomeResourceLoading helper and its folder structure

After those changes, I decided to stop and continue in another PR (the upcoming changes are more extensive than these ones).

Benefits

I don't have any cool before/after shots in this PR :P, but I do have a minor improvement in the useOutsideClick: now it also responds to touch events:

outsideClickTouch.mp4

Possible drawbacks

N/A (hope any!)

Applicable issues

Related to #6563

Additional information

N/A

antgamdia added 18 commits July 28, 2023 14:03
@antgamdia
Remove unused checkbox component
9ab9966 
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@antgamdia
Make SearchFilter use the cds input instead
f2e5582 
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@antgamdia
Remove unused input component
e669dad 
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@antgamdia
Replace all tooltips with react-tooltip instead
b1dd6a3 
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@antgamdia
Remove unused tooltip component
6cfb181 
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@antgamdia
Merge branch 'main' into remove-js-components
dc95185
@antgamdia
Update imports
ea7224e 
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@antgamdia
Update test cases
2bd380f 
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@antgamdia
Fix pending test
d37456f 
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@antgamdia
Update copyright year
b01f040 
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@antgamdia
Move Row and Column to to tsx components
3f5a5e3 
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@antgamdia
Update imports
b52ad98 
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@antgamdia
Update imports
2c402da 
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@antgamdia
Update imports
1ef4ecc 
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@antgamdia
Move useOutsideClick to tsx
26eb6b3 
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@antgamdia
Merge branch 'main' into remove-js-components-1
125e61a 
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@antgamdia
Merge branch 'main'
62d4ccf 
Signed-off-by: Antonio Gamez Diaz <[email protected]>

Conflicts:
	dashboard/src/components/AppList/AppListItem.tsx
	dashboard/src/components/AppView/AccessURLTable/AccessURLTable.tsx
	dashboard/src/components/AppView/AppControls/RollbackButton/RollbackDialog.test.tsx
	dashboard/src/components/Config/PkgRepoList/PkgRepoList.tsx
	dashboard/src/components/SearchFilter/SearchFilter.tsx
	dashboard/src/components/hooks/useOutsideClick/useOutsideClick.test.tsx
	dashboard/src/components/js/hooks/useOutsideClick/useOutsideClick.test.js
@antgamdia
Remove unnecesary isSomeResourceLoading helper
5fb8be4 
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@netlify
Copy link

netlify bot commented Aug 1, 2023 •
edited
Loading

✅ Deploy Preview for kubeapps-dev canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 51f86bc
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/64ccc4f0bd25d700087730ab

@antgamdia antgamdia changed the title Remove js components 1 Remove old js components: moving some logic to typescript Aug 2, 2023
@antgamdia antgamdia mentioned this pull request Aug 2, 2023
Merged
@absoludity
Copy link
Contributor

Nice - thanks Antonio. Let me know if you meant to move this out of draft.

@antgamdia
Copy link
Contributor Author

Thanks! Well, it is actually finished, but given the ongoing progress on the react-router, I was planning to merge this one afterward, I don't want to leave your PR with plenty of merge conflicts :P

absoludity
absoludity approved these changes Aug 4, 2023
View reviewed changes
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Well, it is actually finished, but given the ongoing progress on the react-router, I was planning to merge this one afterward, I don't want to leave your PR with plenty of merge conflicts :P

Thank-you! +1

absoludity
absoludity reviewed Aug 4, 2023
View reviewed changes
dashboard/src/components/AppView/AppView.tsx
Comment on lines -35 to +37
import { parseToString, parseToJS } from "shared/yamlUtils";
import { parseToJS, parseToString } from "shared/yamlUtils";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to add an alphabetic sorting to our prettier or lint so you don't need to clean them up :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it would be a great addition. I've seen https://eslint.org/docs/latest/rules/sort-imports and https://github.com/trivago/prettier-plugin-sort-imports. Will have a look later to see if they work for us.

@antgamdia
Merge branch 'main'
d75320f 
Signed-off-by: Antonio Gamez Diaz <[email protected]>

Conflicts:
	dashboard/src/components/AppView/AppView.tsx
	dashboard/src/components/Catalog/Catalog.tsx
	dashboard/src/components/DeploymentForm/DeploymentForm.tsx
	dashboard/src/components/OperatorInstance/OperatorInstance.tsx
	dashboard/src/components/OperatorList/OperatorList.tsx
	dashboard/src/components/OperatorNew/OperatorNew.tsx
	dashboard/src/components/OperatorView/OperatorView.tsx
	dashboard/src/components/PackageHeader/PackageView.test.tsx
	dashboard/src/containers/ConfigLoaderContainer/ConfigLoaderContainer.ts
	dashboard/src/containers/RoutesContainer/Routes.test.tsx
	dashboard/src/containers/RoutesContainer/Routes.tsx
@antgamdia
Update copyright year
51f86bc 
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@antgamdia antgamdia marked this pull request as ready for review August 4, 2023 09:43
@antgamdia antgamdia merged commit 461d4bc into vmware-tanzu:main Aug 4, 2023
@antgamdia antgamdia deleted the remove-js-components-1 branch August 4, 2023 10:03
antgamdia added a commit that referenced this pull request Aug 7, 2023
@antgamdia
Remove old js components: Spinner, Alert (#6554)
e1a72e6 
### Description of the change

This PR is following up #6550 (and requires it to get merged first). In
this one, the focus is on:

- Remove the `Spinner` old js component in favor of `CdsProgressCircle`.
- Replace the old js `Alert` with the `AlertGroup` component we already
had. It is just a wrapper with some handy configuration of the clarity
components.
- By default, they are now closable. Sometimes we have some transient
errors and I think we can just defer to the user the decision on whether
or not to close them.
- Add a "Cancel" button in a modal (new ns) to keep UX consistency.
- Replace the remaining plain `<button>` (we weren't able to back in
time due to an upstream bug).

Again, this PR is pretty extensive (sorry!)

### Benefits

Again, let me show some visual examples :) (the error messages like
"error.message" are just placeholders while developing, they are removed
in the PR).

<details><summary>Initial emptyKubeapps alert</summary>
<p>


![alert1_BA](https://github.com/vmware-tanzu/kubeapps/assets/11535726/14b3daac-200c-4792-9442-7f0e44cd08f9)


</p>
</details> 
<details><summary>Typical error alert</summary>
<p>


![alert2_BA](https://github.com/vmware-tanzu/kubeapps/assets/11535726/b7047ff2-9253-480c-90a6-9ba318e2ba87)


</p>
</details> 
<details><summary>Error with action (in accordance with the clarity
guidelines)</summary>
<p>


![alert3_(layout)_BA](https://github.com/vmware-tanzu/kubeapps/assets/11535726/05389d01-2a90-4199-9a94-f3ef026910e5)


</p>
</details> 
<details><summary>Error with action (another example)</summary>
<p>


![alert4_(catalog)_BA](https://github.com/vmware-tanzu/kubeapps/assets/11535726/d769ac85-d848-43b7-bd43-e66f61fe32b4)


</p>
</details> 
<details><summary>New update available</summary>
<p>


![alert5_(pkgUpdate)_BA](https://github.com/vmware-tanzu/kubeapps/assets/11535726/32cc072e-b440-44ec-8034-1d78ef8c69ec)


</p>
</details> 
<details><summary>Operators warning</summary>
<p>


![alert6_(operators)_BA](https://github.com/vmware-tanzu/kubeapps/assets/11535726/961b050e-732c-4dd4-bc7f-9f6a634a371f)


</p>
</details> 
<details><summary>General error (note the proper margins now)</summary>
<p>


![alert7_(cloader)_BA](https://github.com/vmware-tanzu/kubeapps/assets/11535726/892da93c-a286-4303-b8c1-12172ba70e1b)


</p>
</details> 
<details><summary>Repos in !kubeappsCluster</summary>
<p>


![alert8_(repos)_BA](https://github.com/vmware-tanzu/kubeapps/assets/11535726/c1fa40ce-421e-480d-b665-4d89ac9b1ada)


</p>
</details> 
<details><summary>Add repo error</summary>
<p>


![alert9_(newRepo)_BA](https://github.com/vmware-tanzu/kubeapps/assets/11535726/62686d27-fec3-4484-ab67-a9f799c5b609)


</p>
</details> 
<details><summary>Error in a modal (note the margins again)</summary>
<p>


![alert10_(rollback)_BA](https://github.com/vmware-tanzu/kubeapps/assets/11535726/a194d2d8-4231-4f70-86bb-726c6ba7b946)


</p>
</details> 


### Possible drawbacks

N/A (hope any!)

### Applicable issues

Related to #6563

### Additional information

After this PR there are just two remaining js components: `Table` and
`Card`. I will work on them soon!

---------

Signed-off-by: Antonio Gamez Diaz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@absoludity absoludity absoludity approved these changes

Assignees
No one assigned
Labels
cla-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@antgamdia @absoludity @vmwclabot