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

Revert "Use icon exit for close buttons" #40546

Merged
merged 2 commits into from
May 6, 2023
Merged

Conversation

obuisard
Copy link
Contributor

@obuisard obuisard commented May 5, 2023

Reverts #40167

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

@brianteeman
Copy link
Contributor

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.

@Fedik
Copy link
Member

Fedik commented May 5, 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.

@wilsonge
Copy link
Contributor

wilsonge commented May 5, 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

@brianteeman
Copy link
Contributor

yes I see that and I agree with @Fedik

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

@wilsonge
Copy link
Contributor

wilsonge commented May 5, 2023

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

@brianteeman
Copy link
Contributor

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

@obuisard
Copy link
Contributor Author

obuisard commented May 5, 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).

@laoneo
Copy link
Member

laoneo commented May 5, 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.

@Fedik

This comment was marked as outdated.

@Fedik
Copy link
Member

Fedik commented May 5, 2023

Well, ignore my last comment :)

@obuisard
Copy link
Contributor Author

obuisard commented May 5, 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.

@sdwjoomla
Copy link
Contributor

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

@brianteeman
Copy link
Contributor

brianteeman commented May 5, 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

@sdwjoomla sdwjoomla merged commit 647870c into 4.3-dev May 6, 2023
@sdwjoomla
Copy link
Contributor

Thanks Olivier

@sdwjoomla sdwjoomla added this to the Joomla! 4.3.2 milestone May 6, 2023
@wilsonge wilsonge deleted the revert-40167-icon-exit branch May 6, 2023 11:57
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.

7 participants