PR-4.3-dev Pending

User tests: Successful: Unsuccessful:

avatar obuisard
obuisard
5 May 2023

Reverts #40167

Should be something that would be handled in 5.0 rather than in a bug fix release.

avatar joomla-cms-bot joomla-cms-bot - change - 5 May 2023
Category Libraries
avatar obuisard obuisard - open - 5 May 2023
avatar obuisard obuisard - change - 5 May 2023
Status New Pending
avatar brianteeman
brianteeman - comment - 5 May 2023

This makes no sense at all. The change you are reverting here is the icon change NOT the text.

When you create a new article then the text has always been Cancel
When you edit an existing article then the text has always been Close

The only thing the pr did was to change the icon that went with the text.

avatar Fedik
Fedik - comment - 5 May 2023

@obuisard this will fix the problem:

return $this->standardButton('cancel', $text, $task)->icon('icon-exit');

This will keep old tookbar-cancell ID but use new icon, from the linked PR.

avatar wilsonge
wilsonge - comment - 5 May 2023

@brianteeman not quite. It changed the button name. By default the icon is ‘icon-$name’). Hence the icon changed too. But the name also changed the ID of the button which caused test failures in 4.0. I think @Fedik ‘s solution here is correct

avatar brianteeman
brianteeman - comment - 5 May 2023

yes I see that and I agree with @Fedik

However Allon is saying that the label has changed #40167 (comment)

avatar wilsonge
wilsonge - comment - 5 May 2023

The label hasn’t changed. @chmst tracked that down in the maintainers group earlier. Allon was wrong about that

avatar brianteeman
brianteeman - comment - 5 May 2023

it was only 30 minutes ago that he said that :(

avatar obuisard
obuisard - comment - 5 May 2023

The fix from Fedir @Fedik is a good solution but looking at it more closely, brought up to the surface some inconsistencies we have in the toolbars (for instance, we have a 'close' button instead of 'cancel' when creating a new category).

avatar laoneo
laoneo - comment - 5 May 2023

Perhaps I did change the branches too fast and the labels are still the same. Then it is my fault. But the other statement from me is still valid. This should not go into a patch release.

avatar Fedik
Fedik - comment - 5 May 2023

That probably result of George PR #39537,
there for existing item:

ToolbarHelper::cancel('category.cancel', 'JTOOLBAR_CLOSE');
// changed to
$toolbar->cancel('category.cancel');

Probably need

$toolbar->cancel('category.cancel', 'JTOOLBAR_CLOSE');

At

Hmhm, I mean with diferent text :)

Or other way around for diferent text when "new item".

avatar Fedik
Fedik - comment - 5 May 2023

Well, ignore my last comment :)

avatar obuisard
obuisard - comment - 5 May 2023

@obuisard this will fix the problem:

return $this->standardButton('cancel', $text, $task)->icon('icon-exit');

This will keep old tookbar-cancell ID but use new icon, from the linked PR.

So we don't forget @Fedik suggested adding:
->buttonClass('btn btn-primary')
so that icons turn blue.

avatar sdwjoomla
sdwjoomla - comment - 5 May 2023

Reverting the close icon change makes sense given that the replacement of the icon enhances the look and is not fixing a broken/non-loading icon. Additionally, further discussion is needed to be more consistent in the actions, using Close vs Cancel.
image

avatar brianteeman
brianteeman - comment - 5 May 2023

Additionally, further discussion is needed to be more consistent in the actions, using Close vs Cancel.

What discussion is needed - this has been the behaviour for a very very long time without issue. The only issue was regarding the icon clashing with the delete icon. Modules dont have the seperate concept of new and edit. Only components do

avatar sdwjoomla sdwjoomla - change - 6 May 2023
Labels Added: PR-4.3-dev
avatar sdwjoomla sdwjoomla - change - 6 May 2023
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2023-05-06 03:27:40
Closed_By sdwjoomla
avatar sdwjoomla sdwjoomla - close - 6 May 2023
avatar sdwjoomla sdwjoomla - merge - 6 May 2023
avatar sdwjoomla
sdwjoomla - comment - 6 May 2023

Thanks Olivier

Add a Comment

Login with GitHub to post a comment