Deprecate decorate*() JS functions

[loekvangool]

Description (*)

JavaScript functions like decorateTable() are popular in the templates. Their purpose is to add CSS classes odd, even, first, last to HTML tables, lists, etc.

This seems wildly outdated and requires browser repainting, leading to lower Web Vitals scores. CSS3 can easily target these entities without writing classes with JavaScript after initial page load.

Note: Even when there are no actual visual changes, Chrome still reports a repaint and deducts points. These functions should not be used.

[fballiano]

I'd love to drop them :-)

[justinbeaty]

Previous discussion about decorate*() functions was here: #1073 (comment)

Maybe instead of removing, we can add a config setting to enable / disable them? Something like a "legacy theme compatibility" switch? That way, you can disable to improve performance if needed.

[loekvangool]

Thanks for referencing #1073 I hadn't found that one yet.

This project requires PHP 7.3 which was released in december 2018. Meanwhile, the CSS discussed in this issue has been supported in Chrome since 2010 (https://caniuse.com/css-sel2). I just wanted to point that out, it seems like a huge difference between how frontend and backend code is viewed by some.

To be brutally honest I don't have these functions on my own template anymore, so I'm good if it stays.

[justinbeaty]

@loekvangool I think the hesitation about removing it from the theme was that perhaps there is JS out there that finds nodes based on these classes and manipulates the DOM expecting these classes.

If nothing else, I'd be happy to make a PR to implement the config in the admin, but I don't even use the default theme either so I wouldn't be able to give it a good testing.

[tmotyl]

Just kill it with fire :)
V20 doesnt have to be backward compatible

[loekvangool]

@loekvangool I think the hesitation about removing it from the theme was that perhaps there is JS out there that finds nodes based on these classes and manipulates the DOM expecting these classes.

Yes that's clear, however we require PHP 7.3 which means all users need to go through their code to fix stuff that does not work anymore. It seems fair to declare a decent interval in which OM expects users to update their code? If you mean code that's part of OM that needs decorate*(), I agree it should be part of the removal.

I just noticed that these functions are in js/, which means they cannot be altered without making a core hack. We should probably support overwriting js/ files?

[justinbeaty]

however we require PHP 7.3 which means all users need to go through their code to fix stuff that does not work anymore. It seems fair to declare a decent interval in which OM expects users to update their code?

Yes, that's fair that users should be expected to maintain their 3rd party modules and I'm certainly not against the removal of these functions -- especially in 20.x. Mainly I wanted to link the previous discussions about it and make sure everything was considered.

I just noticed that these functions are in js/, which means they cannot be altered without making a core hack. We should probably support overwriting js/ files?

If we decide to go the route of a config option, we can set a JS variable like how I did in this PR: https://github.com/OpenMage/magento-lts/pull/2426/files#diff-1949685fb6f678895c467342c6ed455ed44726a6eb66f9dcb0ee9beb07088880

And then in the JS file check something like this at the top of the decorate functions.

function decorateGeneric(elements, decorateParams)
{
    if(window.NO_DECORATE)
        return
    // ...

[tmotyl]

Im against adding config for acient js stuff like this. It just clutters the codebase.

If somebody wants to make frontend in 2023 like it was 2010 then he is on his own.

[fballiano]

I also don't like configuration options, especially for this kind of stuff. ancient stuff needs to go and developers have to know that some work is needed when upgrading (like with every other kind of software)

[justinbeaty]

Then 🔥🔥🔥 it is

[luigifab]

Since #1073, I replaced all decorate functions with (to avoid ReferenceError: decorateXyz is not defined):

function decorateList() {
    console.warn('decorateList is deprecated');
}

[luigifab]

I have a PR for that if interested (the removed part of 1073).

[fballiano]

i'd even just function decorateList() {} ;-)

[fballiano]

I think that, in 2023, we should rethink the decision of #1073 (comment) and drop this funcions in v21