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
Media Manager-validation required on delete button #4777
Conversation
jainrimjhim20
commented
Oct 17, 2014
Merge remote tracking branch 'upstream/staging'
Confirmed. I get a message now :) http://screencast.com/t/XJGLzlTVj |
@test Tested this successfully. |
The issue is related to the usage of JToolBar::appendButton() function in com_media as there is no such boolean parameter available(true or false) which if passed can generate an alert box asking for making a selection from the list. Where as in case of other pre-defined JToolBar buttons, like publish , unpublish etc. the alert popup can be easily implemented by simply passing the boolean parameter. The important point in this case which needs to be enlightened is that the delete button in this issue is not the pre-defined JToolbarHelper::deleteList(); but it is a custom JToolBar::appendButton(); button. So, the work around for this would be customizing the javascript code in the component but not in the library files.I am working on it. |
Based on the last comment I am setting this one as Needs Review by maintainers |
The solution for this one seems to be ok (and I was able to test it successfully as well). The current Travis CI build failure being shown on the Pull Request page for this one seems to be unrelated to the actual patch itself. |
I was able to reproduce the bug: in Media Manager, click the Delete button with nothing selected. It reloads like it's doing something with no error or confirmation of what happened. After applying the patch, tested again and clicking Delete button without anything selected results in error box "Please first make a selection from the list". Bug fixed. |
There is an error in the unittests with your changes. Please see if you can fix those. |
Merge remote tracking branch 'upstream/staging'
I can't see anything wrong with this - removing from needs review. Can I have one test here to confirm the patch is still required and works as expected before it goes RTC please? This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4777. |
@wilsonge is the comment about unit tests no longer valid either? This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4777. |
I don't think so. There are no unit tests failing with this and there are none covering this kinda stuff anyhow. I think Dmitris will throw a small hissy fit about using script tags rather than using JDocument but given the age of this PR i"d rather merge this if it works and then get him to do that separately |
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4777. |
I have tested this item ✅ successfully on 97f89dc This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4777. |
I have tested this item ✅ successfully on 97f89dc Personally this type of functionality should be labelled "you are an idiot why did you click without selecting" This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4777. |
RTC This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4777. |
@rbsl-rimjhim could you check the merge conflicts, thanks |
Robert as this PR is 18 months old think this is one of those cases where we can do it ourselves ;) |
Feel free to do it :-P |
I will do it now |
See #9891 This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4777. |
Merged #9891 |