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

[5.0] Select Schedule Task css #41871

Merged
merged 13 commits into from
Sep 30, 2023
Merged

Conversation

brianteeman
Copy link
Contributor

The select task view is the same as the select module view except it uses its own css file with invalid css.

This PR changes that so that it uses an scss file in exactly the same way as com_modules. This also removes the invalid css and makes it more maintainable. There will be no visible change

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org: will be added when merged

  • No documentation changes for manual.joomla.org needed

The select task view is the same as the select module view except it uses its own css file with invalid css.

This PR changes that so that it uses an scss file in exactly the same way as com_modules. This removes the invalid css and makes it more maintainable.
@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.0-dev labels Sep 23, 2023
@HLeithner
Copy link
Member

removing the asset from the joomla.asset.json could lead to an exception if 3rd party uses it. since we don't have a policy for it. Please move this entry to the b/c plugin with an empty file reference and update the manual b/c / deprecation section.

@brianteeman
Copy link
Contributor Author

removing the asset from the joomla.asset.json could lead to an exception if 3rd party uses it. since we don't have a policy for it. Please move this entry to the b/c plugin with an empty file reference and update the manual b/c / deprecation section.

sorry - not sure exactly what you are asking for here

@HLeithner
Copy link
Member

more or like a copy of that what the compat plugin does for the es5 assets. new json file with the removed parts from this pr and loading the new json file when the es5 assets are loaded

@brianteeman
Copy link
Contributor Author

sorry I think I understand what you mean but I dont know what to put in place of the xxxxxxxxxxx


        /**
         * Load the es5 assets stubs, they are needed if an extension
         * directly uses a core es5 asset which has no function in Joomla 5+
         * and only provides an empty asset to not throw an exception
         */
        if ($this->params->get('es5_assets', '1')) {
            $event->getDocument()
                ->getWebAssetManager()
                ->getRegistry()
                ->addRegistryFile('media/plg_behaviour_compat/es5.asset.json');
        }

        /**
         * Load the com_scheduler.admin-view-select-task-css stub asset, if an extension
         * directly used it - very unlikely - to an asset which has no function in Joomla 5+
         * and only provides an empty asset to not throw an exception
         */
        if ($this->params->get('***************', '1')) {
            $event->getDocument()
                ->getWebAssetManager()
                ->getRegistry()
                ->addRegistryFile('media/plg_behaviour_compat/**********.json');
        }

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators and removed NPM Resource Changed This Pull Request can't be tested by Patchtester labels Sep 26, 2023
@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester and removed Language Change This is for Translators labels Sep 26, 2023
@richard67
Copy link
Member

@brianteeman Shall I help with the merge conflicts?

This reverts commit a9e3bedc2cc97deb806be1138fe0402e30c2e932.
@brianteeman
Copy link
Contributor Author

@richard67 yes please - not in the right headspace today to resolve it

@brianteeman
Copy link
Contributor Author

Any help with this would be appreciated as well #41871 (comment)

@wilsonge @HLeithner @richard67

@wilsonge
Copy link
Contributor

I think like this

if ($this->params->get('es5_assets', '1')) {

@richard67
Copy link
Member

@brianteeman Conflicts resolved. With the other thing I don't know yet if I can help.

@richard67
Copy link
Member

System tests will fail in Drone due to unrelated reasons, see #41938 .

@brianteeman
Copy link
Contributor Author

thanks @richard67

@wilsonge yes I assumed that in my post just dont have a clue what to put in place of the xxxxxxxxxx in my code snippet

@wilsonge
Copy link
Contributor

Create a new parameter in the plugin. Doesn't make sense to mix this with the existing es5 assets

@brianteeman
Copy link
Contributor Author

thats what i am doing arent i? just need to know what to put instead of the xxxxxxxxxxx

/me confused

@wilsonge
Copy link
Contributor

https://github.com/joomla/joomla-cms/blob/5.0-dev/plugins/behaviour/compat/compat.xml#L36-L47 new param in here to match and then pick a name. removed_assets seems reasonable and we can polyfill any others into the same param rather than doing a param per asset.

@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Sep 26, 2023
@brianteeman
Copy link
Contributor Author

Hopefully I just got it right now. As soon as you mentioned the xml it started to click

@wilsonge
Copy link
Contributor

Looks fine to me

@HLeithner HLeithner merged commit debb587 into joomla:5.0-dev Sep 30, 2023
3 checks passed
@brianteeman
Copy link
Contributor Author

thanks

@brianteeman brianteeman deleted the 6_com_scheduler branch September 30, 2023 20:29
heelc29 added a commit to heelc29/joomla that referenced this pull request Oct 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.0-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants