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

aligned loader center while loading pages #674

Merged
merged 2 commits into from
Jul 11, 2023

Conversation

saikiranpatil
Copy link
Contributor

@saikiranpatil saikiranpatil commented Jul 11, 2023

What

Created Loader Component and added it in all the pages

Screenshot

252311966-c8f09628-d519-4e56-a00c-8f2f9b3a327b

Fixes Issue

#668

@VaiTon
Copy link
Member

VaiTon commented Jul 11, 2023

Supersedes #669

Copy link
Member

@VaiTon VaiTon left a comment

Choose a reason for hiding this comment

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

Some minor issues, but otherwise good work 🎉

src/App.jsx Outdated
@@ -45,6 +44,7 @@ const LogoQuestionValidator = React.lazy(() =>
import("./pages/logosValidator/LogoQuestionValidator")
);
const DashBoard = React.lazy(() => import("./pages/logosValidator/DashBoard"));
const Loader = React.lazy(() => import("./pages/loader"));
Copy link
Member

Choose a reason for hiding this comment

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

I would import this statically, as it should be only a few Bytes and using a dynamic import for a fallback seems counterintuitive.

Copy link
Contributor

Choose a reason for hiding this comment

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

@VaiTon there is index.jsx page in pages folder which is importing all the pages ! I think we don't need that file anymore as we are importing everything directly in the app.jsx file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i had imported everywhere statically, this ones left, i will change that

Copy link
Contributor

@Sudhanva-Nadiger Sudhanva-Nadiger Jul 11, 2023

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@Sudhanva-Nadiger let's make that another PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay ! Cool ! Shall I open a issue ?

Copy link
Member

Choose a reason for hiding this comment

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

You could even directly open a PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Umm cool !

Copy link
Contributor

Choose a reason for hiding this comment

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

@VaiTon the pr is ready :)

src/pages/loader/index.jsx Outdated Show resolved Hide resolved
src/pages/loader/index.jsx Outdated Show resolved Hide resolved
@VaiTon VaiTon merged commit b4fed40 into openfoodfacts:master Jul 11, 2023
7 checks passed
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.

3 participants