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.

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

fix(DHIS2-13915): show spinner when an app is being installed #542

Merged
merged 4 commits into from
Dec 15, 2023

Conversation

kabaros
Copy link
Contributor

@kabaros kabaros commented Dec 13, 2023

fixes DHIS2-13915 and DHIS2-15586.

DHIS2-13915

This adds a spinner to the install button in the main app details and the version list

  • the UX for the version list spinner is not great with the button resizing and pushing the table, but it's better than the previous complete lack of feedback - I am open to easy workarounds that improves this UX though!

appmanagement-spinner

appmangement-versions-loader

DHIS2-13915

This PR also fixes DHIS2-15586. This is a workaround in the absence of an ID we can rely on, it matches the app with the installed apps using the name + developer email. The chances of these conflicting are low, and I think this is better than the current situation where users can't tell at all if an app was installed or not (check video in Jira ticket).

appmanagement-fix-after

@kabaros kabaros requested review from a team as code owners December 13, 2023 06:31
@dhis2-bot
Copy link
Contributor

dhis2-bot commented Dec 13, 2023

🚀 Deployed on https://pr-542--dhis2-app-management.netlify.app

@dhis2-bot dhis2-bot temporarily deployed to netlify December 13, 2023 06:33 Inactive
Copy link

@Mohammer5 Mohammer5 left a comment

Choose a reason for hiding this comment

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

Approving this so I'm not blocking the PR in case my feedback won't be addressed until next week when I'm on vacation

src/components/AppDetails/ManageInstalledVersion.js Outdated Show resolved Hide resolved
src/pages/AppHubApp/AppHubApp.js Outdated Show resolved Hide resolved
@kabaros kabaros force-pushed the DHIS2-13915/spinner-when-installing branch 3 times, most recently from 753ce7b to 09d201d Compare December 14, 2023 14:06
@dhis2-bot dhis2-bot temporarily deployed to netlify December 14, 2023 14:08 Inactive
@kabaros
Copy link
Contributor Author

kabaros commented Dec 14, 2023

@tomzemp I encountered the error with dhis/prop-types - this was caused by this project requiring an older version that exposed all PropTypes from prop-types library, but we stopped doing that since v3 - I could have resolved it with adding a resolutions to package.json, but chose to just drop the use of @dhis2/prop-types since we only use it now for special types.

@kabaros kabaros force-pushed the DHIS2-13915/spinner-when-installing branch from 09d201d to d2649d8 Compare December 14, 2023 14:11
@dhis2-bot dhis2-bot temporarily deployed to netlify December 14, 2023 14:13 Inactive
@kabaros kabaros force-pushed the DHIS2-13915/spinner-when-installing branch from d2649d8 to f605167 Compare December 14, 2023 14:26
@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2023

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (f902c36) 13.03% compared to head (0cfa9a3) 13.10%.

Files Patch % Lines
...rc/components/AppDetails/ManageInstalledVersion.js 0.00% 8 Missing and 1 partial ⚠️
src/components/AppDetails/Versions.js 61.53% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #542      +/-   ##
==========================================
+ Coverage   13.03%   13.10%   +0.06%     
==========================================
  Files          23       23              
  Lines         445      458      +13     
  Branches      122      129       +7     
==========================================
+ Hits           58       60       +2     
- Misses        340      350      +10     
- Partials       47       48       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kabaros kabaros force-pushed the DHIS2-13915/spinner-when-installing branch from 84edcd8 to 0cfa9a3 Compare December 14, 2023 14:44
@dhis2-bot dhis2-bot temporarily deployed to netlify December 14, 2023 14:46 Inactive
@tomzemp
Copy link
Member

tomzemp commented Dec 14, 2023

makes sense @kabaros. no issues running for me now 🙏

@kabaros kabaros requested a review from tomzemp December 14, 2023 16:08
@dhis2-bot dhis2-bot temporarily deployed to netlify December 14, 2023 16:21 Inactive
Copy link
Member

@tomzemp tomzemp left a comment

Choose a reason for hiding this comment

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

looks good 🚀

@kabaros kabaros merged commit 8a0eff0 into master Dec 15, 2023
14 checks passed
@kabaros kabaros deleted the DHIS2-13915/spinner-when-installing branch December 15, 2023 08:41
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 100.2.32 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

5 participants