? ? Pending

User tests: Successful: Unsuccessful:

avatar hardik-codes
hardik-codes
10 Apr 2019

This PR adds checkall-toggle to template styles so that it is consistent with other components

Testing Instructions

Open template styles

Expected result

checkall-toggle_new

Actual result

checkall-toggle_old

Documentation Changes Required

None

avatar hardik-codes hardik-codes - open - 10 Apr 2019
avatar hardik-codes hardik-codes - change - 10 Apr 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 10 Apr 2019
Category Administration com_templates
avatar franz-wohlkoenig franz-wohlkoenig - change - 10 Apr 2019
Title
[4.0]Add checkall-toggle to template styles
[4.0] Add checkall-toggle to template styles
avatar franz-wohlkoenig franz-wohlkoenig - edited - 10 Apr 2019
avatar hardik-codes hardik-codes - change - 10 Apr 2019
Labels Added: J4 Issue ?
avatar hardik-codes
hardik-codes - comment - 10 Apr 2019

done @Quy

avatar hardik-codes
hardik-codes - comment - 12 Apr 2019

@alikon @infograf768 @Quy please test this PR

avatar infograf768
infograf768 - comment - 13 Apr 2019

This PR does not take into account the Toolbar
"Default"

If checkall is used, it should not be available i.e should be greyed

avatar hardik-codes
hardik-codes - comment - 13 Apr 2019

@infograf768
But in other components also like, com_menu, it is not "greyed"
toggle-checkall

Isn't this an expected behavior?

avatar infograf768
infograf768 - comment - 13 Apr 2019

The problem Here is that listCheck is boolean, i.e. we can't apparently force selecting only one item.
And, yes, I think we have a similar problem for com_menus when the menu items selected are assigned the same language (or ALL).

avatar infograf768
infograf768 - comment - 13 Apr 2019

For menus, we have an error
Screen Shot 2019-04-13 at 11 52 02

avatar hardik-codes
hardik-codes - comment - 13 Apr 2019

@infograf768 what could be a possible solution then?

avatar infograf768
infograf768 - comment - 13 Apr 2019

I see that if one choses multiple template styles and use the Default button, then it is the one with the smaller id which is set as default. No message.

avatar infograf768
infograf768 - comment - 13 Apr 2019

Therefore, I would just forget adding a checkall for template styles. It is too imprevisible.
We also don't do it for Languages.

avatar hardik-codes
hardik-codes - comment - 13 Apr 2019

Ok, I get your point, then How do I grey out Toolbar "Default", if check all is used?

avatar infograf768
infograf768 - comment - 13 Apr 2019

As I said, I don’t think it is necessary. We don’t really need to use a checkall in this manager

avatar hardik-codes
hardik-codes - comment - 13 Apr 2019

But for the design of this component to be consistent with others, shouldn't this checkall be included?

avatar brianteeman
brianteeman - comment - 13 Apr 2019

The component should have the checkall.I have seen sites with several pages of template styles. And of course for a consistent ui.

@infograf768 is also correct that the "default" button should be disabled when multiple items are checked.

avatar hardik-codes
hardik-codes - comment - 13 Apr 2019

@brianteeman @infograf768 @bembelimen is there any component in which a toolbar button is disabled when multiple items are selected(to be used as a reference)

avatar brianteeman
brianteeman - comment - 13 Apr 2019

the toolbar has changed a lot in j4 so I'm not sure - you would have to check. I think in j3 the "home" button in the menu manager has the "check" functionality you are looking for although it didnt disable the button and showed a message instead

avatar infograf768
infograf768 - comment - 13 Apr 2019

is there any component in which a toolbar button is disabled when multiple items are selected(to be used as a reference)

no. see above a screen shot of what we get for menu items.
I have also explained above the boolean aspect of “listCheck” which allows greying the button when nothing is selected but does not consider the number of ticked items.

One could create a specific query in the model throwing an alert when attempting to create multiple default styles.

avatar infograf768
infograf768 - comment - 14 Apr 2019

We can at least get an alert message by using

			$toolbar->confirmButton('default')
				->text('COM_TEMPLATES_TOOLBAR_SET_HOME')
				->message('COM_TEMPLATES_CONFIRM_SET_DEFAULT')
				->task('styles.setDefault')
				->listCheck(true);

The message would contain a text telling the user to make sure a unique style is chosen to be set as default.

Screen Shot 2019-04-14 at 07 41 53

avatar hardik-codes
hardik-codes - comment - 14 Apr 2019

We can at least get an alert message by using

@infograf768 which class should I include, to use confirmButton?

avatar infograf768
infograf768 - comment - 14 Apr 2019

use Joomla\CMS\Toolbar\Toolbar;

an then in the method

// Get the toolbar object instance
$toolbar = Toolbar::getInstance('toolbar');

avatar infograf768
infograf768 - comment - 14 Apr 2019

btw, this could be done in another PR as using the Toolbar class only for that Toolbar button would be a waist. Also, I have not figured yet how to add an icon there.

avatar hardik-codes hardik-codes - change - 14 Apr 2019
Labels Removed: J4 Issue
avatar joomla-cms-bot joomla-cms-bot - change - 14 Apr 2019
Category Administration com_templates Administration com_templates Language & Strings
avatar infograf768
infograf768 - comment - 14 Apr 2019

Please let's separate the stuff with the view...

avatar hardik-codes
hardik-codes - comment - 14 Apr 2019

@infograf768 I was able to add the icon
Also, I have included the changes suggested by you

avatar infograf768
infograf768 - comment - 14 Apr 2019

As I said, the view needs more changes once we use the Toolbar class.
Let's make this in a separate PR

avatar hardik-codes
hardik-codes - comment - 14 Apr 2019

As I said, the view needs more changes once we use the Toolbar class.
Let's make this in a separate PR

Could you elaborate for what, the separate PR should be?

avatar infograf768
infograf768 - comment - 14 Apr 2019

Could you elaborate for what the separate PR should be?

It would be ONLY for the View.
It would modify most of ToolbarHelper:: to use $toolbar->
I am on it now. Trying to find the way to also do it for duplicate.
In any case we do need a new string also for the Default message.

avatar hardik-codes
hardik-codes - comment - 14 Apr 2019

Could you elaborate for what the separate PR should be?

It would be ONLY for the View.
It would modify most of ToolbarHelper:: to use $toolbar->
I am on it now. Trying to find the way to also do it for duplicate.
In any case we do need a new string also for the Default message.

Should I revert the latest commit?

avatar infograf768
infograf768 - comment - 14 Apr 2019

Should I revert the latest commit?

Yep. Let's keep this PR for its original purpose.

avatar hardik-codes hardik-codes - change - 14 Apr 2019
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 14 Apr 2019
Category Administration com_templates Language & Strings Administration com_templates
avatar infograf768 infograf768 - change - 14 Apr 2019
Labels Removed: ?
avatar hardik-codes
hardik-codes - comment - 14 Apr 2019

@infograf768 kindly mark your test for this PR

avatar infograf768
infograf768 - comment - 14 Apr 2019

I have tested this item successfully on fc20ee4


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24541.

avatar infograf768 infograf768 - test_item - 14 Apr 2019 - Tested successfully
avatar infograf768
infograf768 - comment - 14 Apr 2019

Will make the pr tomorrow for the view.

avatar Quy
Quy - comment - 14 Apr 2019

I have tested this item successfully on 429d0c7


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24541.

avatar Quy Quy - change - 14 Apr 2019
Status Pending Ready to Commit
avatar Quy
Quy - comment - 14 Apr 2019

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24541.

avatar Quy Quy - test_item - 14 Apr 2019 - Tested successfully
avatar infograf768 infograf768 - change - 15 Apr 2019
Labels Added: ?
avatar infograf768
infograf768 - comment - 15 Apr 2019

Please test #24591 concerning the alert.

avatar wilsonge wilsonge - change - 22 Apr 2019
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2019-04-22 22:24:42
Closed_By wilsonge
avatar wilsonge wilsonge - close - 22 Apr 2019
avatar wilsonge wilsonge - merge - 22 Apr 2019
avatar wilsonge
wilsonge - comment - 22 Apr 2019

Thanks!

Add a Comment

Login with GitHub to post a comment