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

Add a loading spinner into product page #7810

Merged
merged 1 commit into from Sep 21, 2017

Conversation

Projects
None yet
8 participants
@fatmaBouchekoua
Contributor

fatmaBouchekoua commented Apr 20, 2017

Questions Answers
Branch? 1.7.2.x
Description? When we use product page actions (duplicate, go to catalog, add new product, save) sometimes it can take a long time to perform. And we don't see if it's still loading or not.
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/BOOM-2254
How to test? Go to the product page and make some actions like adding a new product, generating combinations, deleting combinations ...

@xBorderie xBorderie changed the title from Boom 2254 to Add a loading spinner into product page Apr 20, 2017

@vincentbz vincentbz added this to the 1.7.1.2 milestone Apr 24, 2017

@maximebiloe maximebiloe removed this from the 1.7.1.2 milestone Apr 25, 2017

@maximebiloe maximebiloe added the WIP label Apr 25, 2017

@maximebiloe

This comment has been minimized.

Show comment
Hide comment
@maximebiloe

maximebiloe Apr 25, 2017

Contributor

We need to check the behavior with our UX @Leadesign
You're using a spinner not used anywhere else

Contributor

maximebiloe commented Apr 25, 2017

We need to check the behavior with our UX @Leadesign
You're using a spinner not used anywhere else

@fatmaBouchekoua

This comment has been minimized.

Show comment
Hide comment
@fatmaBouchekoua

fatmaBouchekoua Apr 25, 2017

Contributor

I found this spinner on the ui-kit.

Contributor

fatmaBouchekoua commented Apr 25, 2017

I found this spinner on the ui-kit.

@aleeks aleeks changed the base branch from 1.7.1.x to develop May 18, 2017

@vincentbz vincentbz added this to the 1.7.2.0 milestone Jun 8, 2017

@vincentbz

This comment has been minimized.

Show comment
Hide comment
@vincentbz

vincentbz Jun 13, 2017

Contributor

Hi,

I've seen with Léa, the first spinner with the 3 colored points will be removed as it has never been used.
We would like to use one single spinner which is the circle one

image

The UI Kit page is not up to date on that point, this is why there is this confusion, sorry for that.
Would it be possible to use this one please ?

Thanks !

Contributor

vincentbz commented Jun 13, 2017

Hi,

I've seen with Léa, the first spinner with the 3 colored points will be removed as it has never been used.
We would like to use one single spinner which is the circle one

image

The UI Kit page is not up to date on that point, this is why there is this confusion, sorry for that.
Would it be possible to use this one please ?

Thanks !

@fatmaBouchekoua

This comment has been minimized.

Show comment
Hide comment
@fatmaBouchekoua

fatmaBouchekoua Jun 21, 2017

Contributor

Hello @vincentbz ,

Done !

Contributor

fatmaBouchekoua commented Jun 21, 2017

Hello @vincentbz ,

Done !

@eternoendless

Thanks for those changes! There are still some small things to fix, though.

@maximebiloe maximebiloe modified the milestones: 1.7.2.0, 1.7.2.1 Jun 26, 2017

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Jul 11, 2017

Contributor

Hello @fatmaBouchekoua
I didn't see the spinner on the top left of the Symfony pages.

Contributor

marionf commented Jul 11, 2017

Hello @fatmaBouchekoua
I didn't see the spinner on the top left of the Symfony pages.

@maximebiloe maximebiloe changed the base branch from develop to 1.7.2.x Jul 21, 2017

@aleeks aleeks removed this from the 1.7.2.1 milestone Jul 24, 2017

@fatmaBouchekoua

This comment has been minimized.

Show comment
Hide comment
@fatmaBouchekoua

fatmaBouchekoua Aug 10, 2017

Contributor

Hello @marionf ,
Can you retest please?

Thanks!

Contributor

fatmaBouchekoua commented Aug 10, 2017

Hello @marionf ,
Can you retest please?

Thanks!

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Aug 10, 2017

Contributor

Hello @fatmaBouchekoua
On the Symfony pages, the spinner on the top left of the header is always turning, even when the page is loaded.

Contributor

marionf commented Aug 10, 2017

Hello @fatmaBouchekoua
On the Symfony pages, the spinner on the top left of the header is always turning, even when the page is loaded.

@fatmaBouchekoua

This comment has been minimized.

Show comment
Hide comment
@fatmaBouchekoua

fatmaBouchekoua Aug 10, 2017

Contributor

Hello @marionf ,
There's a javascript error caused by the module Gamification that causes this issue (This PR should fix the problem but it's not merged yet PrestaShop/gamification#15 )
So could you please disable the Gamification module and retest?

Thanks!

EDIT: This PR PrestaShop/gamification#15 has been merged, you can test without disabling the Gamification module

Contributor

fatmaBouchekoua commented Aug 10, 2017

Hello @marionf ,
There's a javascript error caused by the module Gamification that causes this issue (This PR should fix the problem but it's not merged yet PrestaShop/gamification#15 )
So could you please disable the Gamification module and retest?

Thanks!

EDIT: This PR PrestaShop/gamification#15 has been merged, you can test without disabling the Gamification module

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Aug 11, 2017

Contributor

It's ok for me, thank you @fatmaBouchekoua

Contributor

marionf commented Aug 11, 2017

It's ok for me, thank you @fatmaBouchekoua

@eternoendless eternoendless added this to the 1.7.2.2 milestone Aug 14, 2017

@@ -12,6 +12,8 @@
<header>
<nav class="main-header">
<button class="btn btn-primary-reverse onclick btn-lg unbind ajax-spinner"></button>

This comment has been minimized.

@eternoendless

eternoendless Aug 14, 2017

Member

Why is the spinner a button?

@eternoendless

eternoendless Aug 14, 2017

Member

Why is the spinner a button?

This comment has been minimized.

@fatmaBouchekoua

This comment has been minimized.

@eternoendless

eternoendless Aug 14, 2017

Member

I see. However, it looks like that component was designed with the idea of a button that has a "loading" state in mind. For this kind of loader, we shouldn't create an ad-hoc button, I don't believe it makes sense semantically.

Instead, I think we should use the "dynamic mode" spinner, aka "div with a class". I understand that "dots" spinner shouldn't be used. In that case, we should update the UI Kit to replace the dots with the spinning wheel.

@vincentbz @maximebiloe what do you guys think?

@eternoendless

eternoendless Aug 14, 2017

Member

I see. However, it looks like that component was designed with the idea of a button that has a "loading" state in mind. For this kind of loader, we shouldn't create an ad-hoc button, I don't believe it makes sense semantically.

Instead, I think we should use the "dynamic mode" spinner, aka "div with a class". I understand that "dots" spinner shouldn't be used. In that case, we should update the UI Kit to replace the dots with the spinning wheel.

@vincentbz @maximebiloe what do you guys think?

This comment has been minimized.

@maximebiloe

maximebiloe Aug 21, 2017

Contributor

The UI Kit will be updated because a lot of things do not match the mockups. So, for now, I'd say it's okay to have a button.
You can follow the work on the dedicated branch here: https://github.com/PrestaShop/prestashop-ui-kit/tree/develop
I'm also preparing a branch (soon a PR) to integrate this new UI Kit into PrestaShop.

@maximebiloe

maximebiloe Aug 21, 2017

Contributor

The UI Kit will be updated because a lot of things do not match the mockups. So, for now, I'd say it's okay to have a button.
You can follow the work on the dedicated branch here: https://github.com/PrestaShop/prestashop-ui-kit/tree/develop
I'm also preparing a branch (soon a PR) to integrate this new UI Kit into PrestaShop.

@vincentbz

This comment has been minimized.

Show comment
Hide comment
@vincentbz

vincentbz Sep 4, 2017

Contributor

Hi !
Still need a PM feedback on that one ?

Contributor

vincentbz commented Sep 4, 2017

Hi !
Still need a PM feedback on that one ?

@marionf marionf added waiting for QA and removed QA ✔️ labels Sep 13, 2017

@marionf

This comment has been minimized.

Show comment
Hide comment
@marionf

marionf Sep 13, 2017

Contributor

I will re-test the PR after the code review

Contributor

marionf commented Sep 13, 2017

I will re-test the PR after the code review

@mickaelandrieu

Sorry, but you need to rebase, then ping me and I'll recheck again 👍

@fatmaBouchekoua

This comment has been minimized.

Show comment
Hide comment
@fatmaBouchekoua

fatmaBouchekoua Sep 13, 2017

Contributor

Hello @mickaelandrieu ,

Done, can you recheck please?

Thanks !

Contributor

fatmaBouchekoua commented Sep 13, 2017

Hello @mickaelandrieu ,

Done, can you recheck please?

Thanks !

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless
Member

eternoendless commented Sep 21, 2017

Thank you @fatmaBouchekoua

@eternoendless eternoendless merged commit ba23b10 into PrestaShop:1.7.2.x Sep 21, 2017

2 checks passed

codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@prestonBot prestonBot referenced this pull request Sep 23, 2017

Closed

1.7.2.x #8363

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