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

(BC) Removed IE compatibility #1073

Merged
merged 7 commits into from
May 19, 2021
Merged

(BC) Removed IE compatibility #1073

merged 7 commits into from
May 19, 2021

Conversation

luigifab
Copy link
Contributor

@luigifab luigifab commented Jun 29, 2020

Description

I suggest to kill IE 5 / 6 / 7 / 8 / 9 compatibility.
I suggest also to remove the if tag for addItem of Mage_Page_Block_Html_Head.

I don't suggest to maintain compatibility only with 3 latest recent versions of all major browsers.

10,840 deletions, I like that, and I hate IE.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

kkrieger85
kkrieger85 previously approved these changes Jun 29, 2020
Copy link
Contributor

@kkrieger85 kkrieger85 left a comment

Choose a reason for hiding this comment

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

Throw the IE away :D

@colinmollenhour colinmollenhour marked this pull request as ready for review June 29, 2020 23:18
@colinmollenhour
Copy link
Member

Oops, I clicked "Ready for review" accidentally... Should this still be in draft status for some reason?

@sreichel
Copy link
Contributor

I suggest also to kill the if tag for addItem (not yet done).

?

Copy link
Contributor

@damien-biasotto damien-biasotto left a comment

Choose a reason for hiding this comment

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

Next step: Let's get rid of Prototype.js and use plain JS instead :)

The functions provided by prototype are now all supported natively. Ok, fetch is not yet supported by IE11 but we can introduce polyfills for that.

Copy link
Contributor

@damien-biasotto damien-biasotto left a comment

Choose a reason for hiding this comment

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

There are some methods specific to IE in Mage_Page_Block_Html_Head that could be removed too :

addCssIe
addJsIe

@kkrieger85
Copy link
Contributor

@luigifab could you please rebase/fix conflicting file?

@luigifab luigifab marked this pull request as draft July 10, 2020 10:26
@luigifab
Copy link
Contributor Author

luigifab commented Jul 10, 2020

So sorry for my bad commits some days ago (with xmlconnect changes).
Here is a new proposal with more deletions.

I removed decorateGeneric decorateTable decorateList decorateDataList JS functions.
This will probably break some CSS based on .odd .even .first .last classes, not yet investigated/fixed.

@colinmollenhour
Copy link
Member

I removed decorateGeneric decorateTable decorateList decorateDataList JS functions.
This will probably break some CSS based on .odd .even .first .last classes, not yet investigated/fixed.

Let's not do this, it will break BC and honestly I don't think there is much performance impact from using them even though it would not be considered good practice these days.

@Flyingmana
Copy link
Contributor

The decorate table function has a notable impact, as it can force a redraw of many page parts, after this new classes are applied.

But Iam absolutely against removing it. Its not only IE what is affected here, but all browsers, as the themes uses this classes and likely also many css and js people wrote over the years.
This function is not something like a polyfill, it adds new elements which are used, and removing this will for sure break many things.
But for new themes, js and css we should discourage making use of this party.

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Bundle Relates to Mage_Bundle Component: Catalog Relates to Mage_Catalog Component: CatalogInventory Relates to Mage_CatalogInventory Component: CatalogSearch Relates to Mage_CatalogSearch Component: Checkout Relates to Mage_Checkout Component: Oauth Relates to Mage_Oauth Component: Page Relates to Mage_Page Template : admin Relates to admin template Template : base Relates to base template XML Layout labels Jul 24, 2020
Copy link
Contributor Author

@luigifab luigifab left a comment

Choose a reason for hiding this comment

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

So, no change for decorate functions.

@luigifab luigifab marked this pull request as ready for review July 25, 2020 10:31
@github-actions github-actions bot added downloader JavaScript Relates to js/* Template : install Relates to install template Template : rwd Relates to rwd template Template : default Relates to base template labels Jul 25, 2020
Copy link
Member

@colinmollenhour colinmollenhour left a comment

Choose a reason for hiding this comment

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

Excellent work @luigifab !

@fballiano
Copy link
Contributor

I'd love to see this merged

@kkrieger85 kkrieger85 merged commit 4567901 into OpenMage:20.0 May 19, 2021
@github-actions
Copy link
Contributor

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
6 runs  ±0  6 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 4567901. ± Comparison against base commit 7914f77.

@luigifab luigifab deleted the ie-no-more branch June 6, 2021 11:03
@luigifab luigifab restored the ie-no-more branch June 6, 2021 11:15
@luigifab luigifab deleted the ie-no-more branch June 6, 2021 11:16
@Flyingmana
Copy link
Contributor

small warning in advance: this PR will get at least partially reverted soon, as it breaks some usecases involving the <action method="addItem"> where we do not have a replacement for. (Issue follows soon)

@addison74
Copy link
Contributor

We may only have reports if it is used as an Internet Explorer browser. At this time it is official that IE 11 will be removed by June 15, 2022 from all Windows distributions. If I look at the statistics, those who still use IE are very few v11 mainly and a few v9. The market is already divided between Chrome, Edge, Firefox, Brave, Vivaldi, Opera, Safari.

But I agree with @Flyingmana if it is merged then it needs to be tested more so as not to create any issues.

@Flyingmana
Copy link
Contributor

the broken usecase is unrelated to Internet Explorer

@addison74
Copy link
Contributor

On first reading of the code almost all removed refer to IE related files. @Flyingmana could you please indicate where you identified that problems could occur?

@Danieleeffe1
Copy link

Danieleeffe1 commented Sep 2, 2021

Hello

I have read a little about the reasons for these changes.

With reference to the bug reported # 1796 I want to clarify that all the browsers I tested (chrome, firefox, opera, edge) do not load the css file of the customized theme

the custom theme is a direct descendant of RWD

apart from the css file and some modifications of some files the classes have not been touched in any way.

the result is that themes other than rwd are not loaded.

it's a huge problem

ps: the problem recurs also with the 20.0.13 version of openmage

@fballiano
Copy link
Contributor

I think that if you need that level of compatibility you should stay on v19 instead of v20.

@Danieleeffe1
Copy link

Danieleeffe1 commented Sep 2, 2021

Penso che se hai bisogno di quel livello di compatibilità dovresti rimanere su v19 anziché v20.

agree
evolve a project.

but don't you risk losing that initial idea of ​​backwards compatibility to avoid losing years of work and energy poured into Magento 1?

What real (performance) advantage have you obtained by making these changes that are causing problems for web designers?

forgive me

when you say i should stay on 19 what do you mean?

as a user I fearlessly affirm that the instructions to upgrade magento from 19.4.x to? they are difficult to understand and therefore to apply.

have i attempted to upgrade my magento version 1.9.4.4 to? but the results were disastrous.

to get a working OpenMage (20.0.8) I had to perform a fresh installation and everything worked.

if it is possible to switch from openmage 20.x to 19.x I kindly ask you to tell me where are the instructions to do it, thanks

I don't think I'm the only one facing similar problems

@Danieleeffe1
Copy link

sorry, I add one last comment:

if you don't want to fix the bug at least explain how to fix it to us poor web designers.

and I repeat .... as far as I'm concerned I take the RWD clone and change practically only the css and the position of some php nodes based on where I want them to appear ....

Thank you

@fballiano
Copy link
Contributor

backward compatibility is always assured by the v19 branch, which is still maintained as promised ;-)

the benefits is that we can have a maintained code, not thousands of files that none of the contributors know/use anymore that just lay there forever, regarding super old technologies that tie us back to the past.

to switch to the v19 it depends how you installed it in the first place, if you copied the files like it was usually done, then simply overwrite everything with the new/old version, it's not gonna be super clean but still..

@Danieleeffe1
Copy link

Thanks for the reply

my situation is this:

Ubuntu server 20.04
php 7.4
mysql 8

OpenMage 20.0.8 installed via composer and currently updated via composer to 20.0.12.

so: how should I do?

consider me as a child to accompany by the hand.

Thank you

@fballiano
Copy link
Contributor

if you're doing it with composer simply rebuild the composer.json file, delete the composer.lock and the vendor folder and the "composer install"

@OpenMage OpenMage deleted a comment from joshua-bn Aug 17, 2022
@OpenMage OpenMage deleted a comment from joshua-bn Aug 17, 2022
@OpenMage OpenMage deleted a comment from joshua-bn Aug 17, 2022
@kiatng kiatng mentioned this pull request Nov 23, 2022
@justinbeaty justinbeaty mentioned this pull request Jan 14, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Checkout Relates to Mage_Checkout Component: Oauth Relates to Mage_Oauth Component: Page Relates to Mage_Page Component: PayPal Relates to Mage_Paypal JavaScript Relates to js/* Template : admin Relates to admin template Template : base Relates to base template Template : install Relates to install template Template : rwd Relates to rwd template XML Layout
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants