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

Remove deprecated enable_mobile|disable_mobile #27527

Merged
merged 1 commit into from Mar 9, 2022

Conversation

PrestaEdit
Copy link
Contributor

@PrestaEdit PrestaEdit commented Jan 29, 2022

Questions Answers
Branch? develop
Description? Remove the enable_mobile and disable_mobile action from ModuleManager, deprecated since 1.7.3.0
Type? improvement
Category? CO
BC breaks? yes
Deprecations? no
Fixed ticket Relative to #26293
How to test? Use the action on module (enable for mobile
Possible impacts? n/d

📓 BC break

  • Remove method enable_mobile() in src/Core/Addon/Module/ModuleManager.php
  • Remove method disable_mobile() in src/Core/Addon/Module/ModuleManager.php

This change is Reviewable

@PrestaEdit PrestaEdit requested a review from a team as a code owner January 29, 2022 12:17
@prestonBot
Copy link
Collaborator

Hi, thanks for this contribution!

I found some issues with the Pull Request description:

  • Your pull request does not seem to fix any issue, consider creating one (see note below) and linking it by writing Fixes #1234.

Would you mind having a look at it? This will help us understand how interesting your contribution is, thank you very much!

About linked issues

Please consider opening an issue before submitting a Pull Request:

  • If it's a bug fix, it helps maintainers verify that the bug is effectively due to a defect in the code, and that it hasn't been fixed already.
  • It can help trigger a discussion about the best implementation path before a single line of code is written.
  • It may lead the Core Product team to mark that issue as a priority, further attracting the maintainers' attention.

(Note: this is an automated message, but answering it will reach a real human)

@prestonBot prestonBot added develop Branch Improvement Type: Improvement BC break Type: Introduces a backwards-incompatible break labels Jan 29, 2022
@Hlavtox
Copy link
Contributor

Hlavtox commented Jan 31, 2022

@NeOMakinG Cannot we just completely remove this? Its 2010 stuff.

  • Mobile users should have the same experience as desktop.
  • We have responsivity.
  • What even defines mobile and desktop? We have an infinite range of screen sizes.

@NeOMakinG
Copy link

@NeOMakinG Cannot we just completely remove this? Its 2010 stuff.

  • Mobile users should have the same experience as desktop.
  • We have responsivity.
  • What even defines mobile and desktop? We have an infinite range of screen sizes.

I agree, responsive should be enough, this is not the backend responsibility

@kpodemski
Copy link
Contributor

@NeOMakinG @Hlavtox

But this prevents the module from running on mobile devices, which means that if you have a heavy module connecting to the external service, and you want to use it on desktop, and not on mobile - you can. It's completely different than hiding/showing something on mobile.

@Hlavtox
Copy link
Contributor

Hlavtox commented Feb 3, 2022

No module should be that heavy that it should not be loaded on mobile. Usually over 60% of all customers access the web from mobile devices, they need to have the same experience as on desktop. ;-)

@kpodemski
Copy link
Contributor

Let's leave that decision on what customers should, or shouldn't have to the developers 👍 I can imagine some scenarios, especially related to the B2B stores where this feature can be helpful :-)

@kpodemski kpodemski added this to the 8.0.0 milestone Feb 3, 2022
@Progi1984 Progi1984 added the Waiting for QA Status: action required, waiting for test feedback label Mar 3, 2022
@AureRita AureRita self-assigned this Mar 9, 2022
Copy link
Contributor

@AureRita AureRita left a comment

Choose a reason for hiding this comment

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

I tested it and the variable seems to be correctly changed,

as we can see on this picture

the module manager have 75 utilisation of disableMobile (on Mozilla)
image

It seems to be the same on google
image

there is no use of _mobile
image

@AureRita AureRita added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Mar 9, 2022
@NeOMakinG NeOMakinG merged commit 8a4370b into PrestaShop:develop Mar 9, 2022
@NeOMakinG
Copy link

Thanks @PrestaEdit

I really think that this PR will create a lot of wrong behaviors on different modules, be prepared to get some issues about it!

@PrestaEdit
Copy link
Contributor Author

Doesn't see why, as you are still able to enable/disable mobile devices for somes modules. It's just the old methods (not used by module itself) that are removed, not the feature.

@NeOMakinG
Copy link

Doesn't see why, as you are still able to enable/disable mobile devices for somes modules. It's just the old methods (not used by module itself) that are removed, not the feature.

Oh ok, thanks for the precision!

@matks matks added Documentation ✔️ Developer documentation is up-to-date Needs documentation Needs an update of the developer documentation and removed Documentation ✔️ Developer documentation is up-to-date labels Apr 1, 2022
@matks
Copy link
Contributor

matks commented Apr 1, 2022

I think we also need to document BC break inside https://devdocs.prestashop.com/8/modules/core-updates/8.0/ so I keep the label "Needs documentation"

@kpodemski kpodemski added Documentation ✔️ Developer documentation is up-to-date and removed Needs documentation Needs an update of the developer documentation labels Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC break Type: Introduces a backwards-incompatible break develop Branch Documentation ✔️ Developer documentation is up-to-date Improvement Type: Improvement QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants