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

[4.0] [IAM-sprint] Remove inline scripts part I #14710

Merged
merged 2 commits into from
Mar 18, 2017
Merged

[4.0] [IAM-sprint] Remove inline scripts part I #14710

merged 2 commits into from
Mar 18, 2017

Conversation

dgrammatiko
Copy link
Contributor

Redo of #12399 .

Summary of Changes

Remove inline scripts:

  • submitbutton is beefed up in core.js
  • editor save is controlled by the editor (editor.on('submit', ...)
  • permissions override is controlled by the permissions.js

Testing Instructions

Test that article save, save and new, save and close and cancel still works
Test that http://j4a.dev/administrator/index.php?option=com_config buttons: save, save and close, cancel still work as expected

Expected result

Same as before

Actual result

Same as before

Documentation Changes Required

There is one extra data attribute in the form tag named data-cancel-task="string", which is required only for forms that the cancel task is not the default e.g. controllerName.cancel.
This will not affect any 3rd party app, the override still works as before 100% B/C !!!

@@ -78,8 +78,41 @@ Joomla.editors.instances = Joomla.editors.instances || {
/**
* Default function. Usually would be overriden by the component
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this comment needs changing as well now ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It still can be removed by the component ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

but its not "usually" any more :)

@ghost
Copy link

ghost commented Mar 18, 2017

Save result in Error:
bildschirmfoto 2017-03-18 um 11 48 11

@dgrammatiko
Copy link
Contributor Author

@franz-wohlkoenig that's unrelated to this PR, this is a PHP error meaning that the js functioned as expected (the form was submitted) but for some reason the actual save failed.
So although you got an error you have a successful test :)

@wilsonge wilsonge merged commit 43ecdc9 into joomla:4.0-dev Mar 18, 2017
@wilsonge wilsonge added this to the Joomla 4.0 milestone Mar 18, 2017
@Fedik
Copy link
Member

Fedik commented Mar 19, 2017

here was merged 3 issue:

data-cancel-task="string"

It is wrong approach, really wrong. And it need just for skip validation, and nothing more.
Then would be better to improve the toolbar buttons, to allow to define "which action should be validated", see example: standard.php#L96 _getCommand
Instead of use data-cancel-task hack

permissions in the core

why it here at all?
it can be moved to permissions.js easily, see another example permissions.js#L136

var form = document.querySelectorAll( 'form.form-validate' );

this can return multiple forms, BUT you cannot submit them all (without ajax), always will be submitted only last from the array.
Possible solution is use generic id for the content forms, as it was in the past with adminForm id.

@dgrammatiko
Copy link
Contributor Author

@Fedik the permission part has been already moved to permissions.js
The cancel part needs some touches to the toolbar as the plan is to remove all the crappy inline scripts then that will be fixed there.
For the third issue I guess a simple if id===adminForm will do
The javascript is a WIP, feel free to PR other solution if you feel that the proposed one is not enough ;)

@dgrammatiko dgrammatiko deleted the remove-inline-submitbutton-editorsave-permissions-js branch March 19, 2017 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants