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

feat: Add app loader #17558

Merged
merged 5 commits into from
Jun 17, 2024
Merged

feat: Add app loader #17558

merged 5 commits into from
Jun 17, 2024

Conversation

przemvs
Copy link
Contributor

@przemvs przemvs commented Jun 12, 2024

https://wearezeta.atlassian.net/browse/WPB-9719

Description

Display loading screen while app is not yet initialized. We displays also information while request takes more than 30s.

Screenshots/Screencast (for UI changes)

2024-06-12.11-53-08.mp4

For video delay for show message was changed to 6s.

Checklist

  • PR has been self reviewed by the author;
  • Hard-to-understand areas of the code have been commented;
  • If it is a core feature, unit tests have been added;

Important details for the reviewers

(Delete this section if unnecessary)

  • use (x) data
  • can be reviewed commit-by-commit
  • be sure to look at ...

@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.12%. Comparing base (7fe2553) to head (f82e63a).
Report is 41 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #17558      +/-   ##
==========================================
- Coverage   46.31%   46.12%   -0.19%     
==========================================
  Files         758      760       +2     
  Lines       24904    25012     +108     
  Branches     5714     5724      +10     
==========================================
+ Hits        11535    11538       +3     
- Misses      11928    12031     +103     
- Partials     1441     1443       +2     

@przemvs przemvs marked this pull request as ready for review June 14, 2024 11:12
@przemvs przemvs requested review from otto-the-bot and a team as code owners June 14, 2024 11:12
src/page/index.ejs Outdated Show resolved Hide resolved
src/page/index.ejs Outdated Show resolved Hide resolved
Comment on lines 64 to 70
const removeLoaderRoot = () => {
const loaderRoot = document.getElementById('loader-root');
const loaderStyles = document.getElementById('loader-styles');

loaderRoot?.parentNode?.removeChild(loaderRoot);
loaderStyles?.parentNode?.removeChild(loaderStyles);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

why not putting the loader inside wire-app and let react override it completely?
This way you won't have to write this code at all

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could even put the style tag in there and it will be overwritten by react

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed! :)

const userLang = navigator.language;

setTimeout(() => {
if (loadingMessage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's do the getElementById here, else we are going to have a memory leak and never release the reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link

sonarcloud bot commented Jun 17, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@przemvs przemvs merged commit c678bb8 into dev Jun 17, 2024
11 checks passed
@przemvs przemvs deleted the feat/add-app-loader branch June 17, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants