[4.0] Replace js alerts for joomla alerts #15315

NunoLopesPT · 2017-04-14T22:33:13Z

Summary of Changes

Replace the JS alert() for the Joomla alerts in standart buttons where are no boxes checked

Testing Instructions

Go Components -> Banners -> Banners

Without any box checked, hit publish item

Expected result

Also, I think the close button is white because the class is not completed, here is where the class is called:
https://github.com/joomla/joomla-cms/blob/4.0-dev/media/system/js/core.js#L333

Actual result

Documentation Changes Required

none

zero-24 · 2017-04-14T22:54:01Z

/home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/cms/pagination/JPaginationTest.php:749
5) JToolbarButtonTest::testRender
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
 '
-<button onclick="if (document.adminForm.boxchecked.value == 0) { alert(Joomla.JText._('JLIB_HTML_PLEASE_MAKE_A_SELECTION_FROM_THE_LIST')); } else { Joomla.submitbutton(''); }" class="btn btn-sm btn-outline-primary">
+<button onclick="if (document.adminForm.boxchecked.value == 0) { Joomla.renderMessages({'warning': ['JLIB_HTML_PLEASE_MAKE_A_SELECTION_FROM_THE_LIST', '<h4>Warning</h4>']}) } else { Joomla.submitbutton(''); }" class="btn btn-sm btn-outline-primary">
 	<span class="icon-test"></span>
 	</button>
 '

https://travis-ci.org/joomla/joomla-cms/jobs/222232149

Looks good to me 👍 and is working well. Can you fix the travis error above. The other ones are not realted to your PR. I'm going to open a new issue for that.

C-Lodder · 2017-04-15T00:01:51Z

@NunoLopes96 - that last commit you just made in incorrect. This is PHP, not JS ;)

zero-24 · 2017-04-15T00:12:09Z

@NunoLopes96 in Order to fix the test you need to edit the test itself your code was correct before the last commit ;)

NunoLopesPT · 2017-04-15T00:14:49Z

Ok I'm sorry never used travis before I need to research first a little bit, any tip?

zero-24 · 2017-04-15T00:29:07Z

@NunoLopes96 please check this file + line in your branch: https://github.com/NunoLopes96/joomla-cms/blob/replaceJSalerts/tests/unit/suites/libraries/cms/toolbar/JToolbarButtonTest.php#L126

There is the expeced value stored. So this value needs to be changed as we want to change the output.

…la-cms into replaceJSalerts

NunoLopesPT · 2017-04-15T01:30:02Z

@zero-24 I just changed that, that error no longer appears on travis, thank you for the help

jreys · 2017-04-15T01:37:50Z

I have tested this item ✅ successfully on 746f8d1

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15315.}

rjcf18 · 2017-04-15T01:46:18Z

I have tested this item ✅ successfully on 746f8d1

The Joomla alert appears normally after clicking publish item with no item selected after the patch.

Before patch:

After patch:

System information

Joomla! 4.0.0-dev
OS: Linux Deepin 15.3
Kernel: 4.4.0-3-deepin-amd64
Google Chrome: 56.0.2924.76 (64 bit)
PHP: 7.0.12-2

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15315.}

ghost · 2017-04-15T05:44:51Z

RTC after two successful tests.

joomdonation · 2017-04-15T06:40:31Z

Please remove RTC label. You cannot simply change JText::script to JText::_ as look like JText::script has some code to deal with special character in string

For example, with this change, you can edit the file , change administrator\language\en-GB\en-GB.lib_joomla.ini, change the language item

JLIB_HTML_PLEASE_MAKE_A_SELECTION_FROM_THE_LIST="Please first make a selection from the list."

To:

JLIB_HTML_PLEASE_MAKE_A_SELECTION_FROM_THE_LIST="Please first ' make a selection from the list."

(note the special character ' in the change string). You will get javascript error. Note that the original code works well with this special character in language string

Fedik · 2017-04-15T07:53:53Z

libraries/cms/toolbar/button/standard.php


 		$cmd = "Joomla.submitbutton('" . $task . "');";

 		if ($list)
 		{
-			$alert = "alert(Joomla.JText._('JLIB_HTML_PLEASE_MAKE_A_SELECTION_FROM_THE_LIST'));";
+			$messages = "{'warning': ['" . $alert . "', '<h4>" . JText::_('WARNING') . "</h4>']}";


do you sure it will not fail when the translation contain the quote?
and why do not use Joomla.JText._('JLIB_HTML_PLEASE_MAKE_A_SELECTION_FROM_THE_LIST')?

please use JText::script and do not do this trap, with hard mix of PHP and JavaScript

@Fedik Thank you for the tip, while using your advice I found that no '<h4>" . JText::_('WARNING') . "</h4>' was required if I had a JText::script('WARNING');

Fedik · 2017-04-15T07:58:28Z

tests/unit/suites/libraries/cms/toolbar/JToolbarButtonTest.php

+		$alert = JText::_('WARNING');
+
+		$expected = "\n<button onclick=\"if (document.adminForm.boxchecked.value == 0) { Joomla.renderMessages({'warning': ['JLIB_HTML_PLEASE_MAKE_A_SELECTION_FROM_THE_LIST', '<h4>" 
+			. $alert 


dgrammatiko · 2017-04-15T10:06:51Z

I am against this.
There should not be any inline scripts in J4 in order to satisfy CSP requirements.
So we have to create a toolbar.js file with the logic needed and then pass form the PHP side the config options, e.g. JFactory->getDocument()->addScriptOptions('Toolbar', $optionsArray).

Please DO NO MERGE this!
It's not the right approach!

brianteeman · 2017-04-15T11:07:35Z

CSP? On 15 Apr 2017 11:06 a.m., "Dimitri Grammatikogianni" < notifications@github.com> wrote: I am against this. There should not be any inline scripts in J4 in order to satisfy CSP requirements. So we have to create a toolbar.js file with the logic needed and then pass form the PHP side the config options, e.g. JFactory->getDocument()->addScriptOptions('Toolbar', $optionsArray). Please DO NO MERGE this! It's not the right approach! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#15315 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABPH8W1W4nie8QvQ5Ds6PfUC2CeddN1Tks5rwJa9gaJpZM4M-JnG> .

dgrammatiko · 2017-04-15T11:57:53Z

CSP: https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP
and
#14246

brianteeman · 2017-04-15T12:08:53Z

Thanks. I understand exactly your comment now. However you can have a CSP that allows inline scripts.

…

On 15 Apr 2017 12:57 p.m., "Dimitri Grammatikogianni" < ***@***.***> wrote: CSP: https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#15315 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABPH8XURxb3k_eU8vsjknQhSYWEeMwo-ks5rwLDCgaJpZM4M-JnG> .

…</h4>, travis test changed

NunoLopesPT · 2017-04-15T13:01:59Z

Replaced the JText::_ for JText::script and while doing that found that when adding the JText::script('WARNING'); appeared 2x Warning, so I removed the text with <h4> tags and now the title is higher:

Actual:

Before:

wilsonge · 2017-04-15T15:48:59Z

administrator/language/en-GB/en-GB.lib_joomla.ini

@@ -431,7 +431,7 @@ JLIB_HTML_MOVE_UP="Move Up"
 JLIB_HTML_NO_PARAMETERS_FOR_THIS_ITEM="There are no parameters for this item."
 JLIB_HTML_NO_RECORDS_FOUND="No records found."
 JLIB_HTML_PAGE_CURRENT_OF_TOTAL="Page %s of %s"
-JLIB_HTML_PLEASE_MAKE_A_SELECTION_FROM_THE_LIST="Please first make a selection from the list."
+JLIB_HTML_PLEASE_MAKE_A_SELECTION_FROM_THE_LIST="Please first ' make a selection from the list."


I don't think this should be here

NunoLopesPT · 2017-04-15T17:56:36Z

Sorry, done

wilsonge · 2017-04-15T20:26:09Z

Merged - thanks :)

replace js alerts for joomla alerts

2df5ec7

joomla-cms-bot added the PR-4.0-dev label Apr 14, 2017

NunoLopesPT changed the title ~~replace js alerts for joomla alerts~~ [4.0] Replace js alerts for joomla alerts Apr 14, 2017

zero-24 added this to the Joomla 4.0 milestone Apr 14, 2017

C-Lodder approved these changes Apr 14, 2017

View reviewed changes

Changed JText to see if pass Travis

de953ff

Reverted changes

56c0dd0

NunoLopesPT added 2 commits April 15, 2017 01:42

fixed travis error

6ae685f

Merge branch 'replaceJSalerts' of https://github.com/NunoLopes96/joom…

368f9f1

…la-cms into replaceJSalerts

joomla-cms-bot added the Unit/System Tests label Apr 15, 2017

NunoLopesPT added 3 commits April 15, 2017 01:59

Travis error

ef1d631

\n and \t are not working in , trying different ways

c90bfed

Codestyle

746f8d1

joomla-cms-bot removed this from the Joomla 4.0 milestone Apr 15, 2017

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 15, 2017

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Apr 15, 2017

Fedik reviewed Apr 15, 2017

View reviewed changes

NunoLopesPT added 2 commits April 15, 2017 13:51

Removed JText::_, Added JText::script, no need now to use <h4>Warning…

64b6b80

…</h4>, travis test changed

replaced JLIB_HTML_PLEASE_MAKE_A_SELECTION_FROM_THE_LIST

e6af6dc

joomla-cms-bot added the Language Change This is for Translators label Apr 15, 2017

Travis error

3c2d421

wilsonge reviewed Apr 15, 2017

View reviewed changes

Replace line in lang

76740e0

joomla-cms-bot removed the Language Change This is for Translators label Apr 15, 2017

Remove unneeded variable

5854293

wilsonge merged commit e4c212a into joomla:4.0-dev Apr 15, 2017

wilsonge added this to the Joomla 4.0 milestone Apr 15, 2017

rjcf18 mentioned this pull request Apr 16, 2017

[4.0] Replaced remaining js alerts for joomla alerts for the toolbar buttons #15330

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4.0] Replace js alerts for joomla alerts #15315

[4.0] Replace js alerts for joomla alerts #15315

NunoLopesPT commented Apr 14, 2017 •

edited by joomla-cms-bot

zero-24 commented Apr 14, 2017

C-Lodder commented Apr 15, 2017

zero-24 commented Apr 15, 2017

NunoLopesPT commented Apr 15, 2017

zero-24 commented Apr 15, 2017

NunoLopesPT commented Apr 15, 2017

jreys commented Apr 15, 2017

rjcf18 commented Apr 15, 2017 •

edited

ghost commented Apr 15, 2017

joomdonation commented Apr 15, 2017 •

edited

Fedik Apr 15, 2017

Fedik Apr 15, 2017

NunoLopesPT Apr 15, 2017 •

edited

Fedik Apr 15, 2017

dgrammatiko commented Apr 15, 2017 •

edited

brianteeman commented Apr 15, 2017 via email

dgrammatiko commented Apr 15, 2017 •

edited

brianteeman commented Apr 15, 2017 via email

NunoLopesPT commented Apr 15, 2017 •

edited

wilsonge Apr 15, 2017

NunoLopesPT commented Apr 15, 2017

wilsonge commented Apr 15, 2017

[4.0] Replace js alerts for joomla alerts #15315

[4.0] Replace js alerts for joomla alerts #15315

Conversation

NunoLopesPT commented Apr 14, 2017 • edited by joomla-cms-bot

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

zero-24 commented Apr 14, 2017

C-Lodder commented Apr 15, 2017

zero-24 commented Apr 15, 2017

NunoLopesPT commented Apr 15, 2017

zero-24 commented Apr 15, 2017

NunoLopesPT commented Apr 15, 2017

jreys commented Apr 15, 2017

rjcf18 commented Apr 15, 2017 • edited

Before patch:

After patch:

System information

ghost commented Apr 15, 2017

joomdonation commented Apr 15, 2017 • edited

Fedik Apr 15, 2017

Choose a reason for hiding this comment

Fedik Apr 15, 2017

Choose a reason for hiding this comment

NunoLopesPT Apr 15, 2017 • edited

Choose a reason for hiding this comment

Fedik Apr 15, 2017

Choose a reason for hiding this comment

dgrammatiko commented Apr 15, 2017 • edited

brianteeman commented Apr 15, 2017 via email

dgrammatiko commented Apr 15, 2017 • edited

brianteeman commented Apr 15, 2017 via email

NunoLopesPT commented Apr 15, 2017 • edited

wilsonge Apr 15, 2017

Choose a reason for hiding this comment

NunoLopesPT commented Apr 15, 2017

wilsonge commented Apr 15, 2017

NunoLopesPT commented Apr 14, 2017 •

edited by joomla-cms-bot

rjcf18 commented Apr 15, 2017 •

edited

joomdonation commented Apr 15, 2017 •

edited

NunoLopesPT Apr 15, 2017 •

edited

dgrammatiko commented Apr 15, 2017 •

edited

dgrammatiko commented Apr 15, 2017 •

edited

NunoLopesPT commented Apr 15, 2017 •

edited