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

Move scroll forcing from core to classic #11534

Merged
merged 7 commits into from Jan 16, 2019

Conversation

Projects
None yet
6 participants
@dennispw
Copy link
Contributor

dennispw commented Nov 28, 2018

Questions Answers
Branch? develop
Description? When using the pagination or changing filters (using the ps_facetedsearch module as an example), the user is scrolled to the top of the document. This is considered bad practice since you are taking the control out of the users hands. It could be useful when using the pagination as long as the pagination is located at the bottom of the page but other than that I would really reconsider this functionality. As an example, you often have to scroll down a bit to see the products and any sorting options or filters. Let's say you wanted to selec the "Size: XL" filter. You scroll down, find it and unfortunately you hit the "Size: L" by mistake. Now you're back at the top of the page and have to scroll down yet again. Another example, let's say you want to filter on "Color: Blue" and "Color: Red". You have to scrolldown twice for each selection. It get's even worse if you're on a mobile device where the total length of the content is usually longer. Every single input made by the user will most likely result in having to scroll down - don't take the control out of the users hands unless absolutely necessary. This is something each individual shop owner and/or theme should be in control of and not the core platform.
Type? improvement
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #10993

This change is Reviewable

@prestonBot

This comment has been minimized.

Copy link
Collaborator

prestonBot commented Nov 28, 2018

Hello @dennispw!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@marionf marionf self-assigned this Nov 28, 2018

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Nov 28, 2018

(Assets generated)

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Nov 29, 2018

Hello @dennispw

With your PR, when you change page you don't arrive on the top of the page and you have to scroll up
About faceted search, I have to scroll up also to see all products, which is not ideal
https://drive.google.com/file/d/184T-z2401Sy91lNLZsDFI_mGspKCbeOv/view?usp=sharing

There will always be cases where it will be necessary to scroll up or down according to the number of products displayed on the page, the number of filters configured in faceted search, the device used ...

So, we talked about this with @TristanLDD & @colinegin and we don't want to change the current behavior

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Nov 29, 2018

There is something interesting in the original description:

This is something each individual shop owner and/or theme should be in control of and not the core platform.

@dennispw If you take some time to move the code from the core to the classic theme, I guess we can find a compromise. :)
We keep the feature in the default theme, while theme developer can create their own while choosing to keep this scroll (or not).

@dennispw

This comment has been minimized.

Copy link
Contributor Author

dennispw commented Nov 30, 2018

As far as I can tell, this change should result in the same behaviour as having it in the core.

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Nov 30, 2018

Well, not really, as you can replace the theme assets (and remove the calls to the scroll) with using child theme feature.
Developers who create their module from scratch won't have this call anymore as well. By having it in the core/ folder, everybody has to deal with it. From now, they can take the liberty to remove it.

@dennispw

This comment has been minimized.

Copy link
Contributor Author

dennispw commented Nov 30, 2018

Well yes, bad phrasing on my part. The behaviour should be the same as if it was in the core (as in scrolling to the top) while having the option to remove it by modifying theme assets instead.

@dennispw dennispw changed the title Remove scroll forcing Move scroll forcing from core to classic Nov 30, 2018

});

$('body').on('change', '#search_filters select', function (event) {
const form = $(event.target).closest('form');
prestashop.emit('updateFacets', '?' + form.serialize());
window.scrollTo(0, 0);
});

prestashop.on('updateProductList', (data) => {

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Nov 30, 2018

Member

What do you think about having window.scrollTo(0, 0); inside prestashop.on('updateProductList', (data) => { instead?

This comment has been minimized.

@dennispw

dennispw Nov 30, 2018

Author Contributor

I can absolutely do that instead. However, the bahaviour will be slightly different.

  • Right now, it will render the change (i.e. filter, sort order, pagination, etc) and then scroll to the top.
  • If inside the event, it will scroll to the top as soon as possible without "waiting" for the product list to be completely updated and rendered.

What do you think?

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Dec 3, 2018

Member

I understand your concern, but does it change from the original version?

Let's ping for @TristanLDD about this issue, but if this is the same behavior from the core assets, I would be fine with it.

This comment has been minimized.

@dennispw

dennispw Dec 4, 2018

Author Contributor

It's essentially the same thing, the only thing that differs is when the scroll occurs. After or before the change is set.

One plus on having it the event is DRY code though.

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Dec 4, 2018

Member

Yeah that was the main reason to suggest this update. Thanks for taking the time by the way.

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Dec 4, 2018

PR updated to generate theme classic assets.

@marionf marionf added QA ✔️ and removed waiting for QA labels Dec 4, 2018

@dennispw

This comment has been minimized.

Copy link
Contributor Author

dennispw commented Dec 4, 2018

Is there anything else you need from me regarding this?

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Dec 4, 2018

Not for now. We're waiting for a validation from the UX team before going further.

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jan 15, 2019

Hey @dennispw are you able to rebase your work?

@dennispw

This comment has been minimized.

Copy link
Contributor Author

dennispw commented Jan 16, 2019

@PierreRambaud Could you give me a quick run down on how I would go about doing so?

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jan 16, 2019

@dennispw A good tutorial about how to rebase a branch https://blog.algolia.com/master-git-rebase/
(We use develop and not master as they do 😄 )

@dennispw

This comment has been minimized.

Copy link
Contributor Author

dennispw commented Jan 16, 2019

@PierreRambaud I'm in uncharted territory here and can not get the rebase working properly. How do we proceed?

@Quetzacoalt91
Copy link
Member

Quetzacoalt91 left a comment

Rebase done

@PierreRambaud PierreRambaud added this to the 1.7.6.0 milestone Jan 16, 2019

@Quetzacoalt91 Quetzacoalt91 merged commit ca3bf75 into PrestaShop:develop Jan 16, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Jan 16, 2019

Thank you @dennispw

@dennispw

This comment has been minimized.

Copy link
Contributor Author

dennispw commented Jan 17, 2019

Thank you!

@dennispw dennispw deleted the dennispw:10993 branch Jan 18, 2019

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