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

Use icon exit for close buttons #40167

Merged
merged 2 commits into from
May 4, 2023
Merged

Use icon exit for close buttons #40167

merged 2 commits into from
May 4, 2023

Conversation

chmst
Copy link
Contributor

@chmst chmst commented Mar 22, 2023

Pull Request for Issue #40112 .

Summary of Changes

Replace the "close" icon ( X ) by an exit icon. The exit icon is used in many apps.

Testing Instructions

All toobars have the exit-icon on the close button.

Actual result BEFORE applying this Pull Request

see #40112

Expected result AFTER applying this Pull Request

grafik

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@brianteeman
Copy link
Contributor

The problem with this PR is that we now have multiple icons being used for the same thing. As we are giving the icon a semantic meaning so that people will recognise the icon and know what it does we should be consistent.

@chmst
Copy link
Contributor Author

chmst commented Mar 22, 2023

I think that the exit icon has a meaning: leave the screen and do nothing. The "x" has the meaning "delete" or "cancel"but does not say "leave".

@brianteeman
Copy link
Contributor

My apologies I didnt look properly at the pr only at the reporting issue.

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 11e73c7


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

@chmst chmst added the Small A PR which only has a small change label Mar 22, 2023
@ChristineWk
Copy link

ChristineWk commented Mar 22, 2023

Screenshot 2023-03-22 104241
I have tested this item ✅ successfully on 11e73c7


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

@Fedik Fedik removed PR-4.3-dev UI/UX Small A PR which only has a small change labels Mar 22, 2023
@Fedik
Copy link
Member

Fedik commented Mar 22, 2023

r2c


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 22, 2023
@Fedik Fedik added PR-4.3-dev UI/UX Small A PR which only has a small change labels Mar 22, 2023
@obuisard
Copy link
Contributor

obuisard commented Mar 22, 2023

So much better...


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

However I was thinking that this icon is often used to portray an 'exit' from an application or a logout. Hope this won't confuse some users.

@chmst
Copy link
Contributor Author

chmst commented Mar 22, 2023

However I was thinking that this icon is often used to portray an 'exit' from an application or a logout. Hope this won't confuse some users.

Hopefully ... in this case it is an exit from a screen and together with the text it should be clear. Maybe we should use the close button without icon.


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

@brianteeman
Copy link
Contributor

brianteeman commented Mar 23, 2023

I have tested this item 🔴 unsuccessfully on 11e73c7

Sorry - reverting my successful test to unsuccessful as I didnt spot that the text didnt change as shown in he screenhot and remains as cancel


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

image

@Fedik Fedik removed RTC This Pull Request is Ready To Commit PR-4.3-dev UI/UX Small A PR which only has a small change labels Mar 24, 2023
@Fedik
Copy link
Member

Fedik commented Mar 24, 2023

back to pending


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

@Fedik Fedik added PR-4.3-dev UI/UX Small A PR which only has a small change labels Mar 24, 2023
@drmenzelit
Copy link
Contributor

the text didnt change as shown in he screenhot and remains as cancel

But "close" would be wrong on this place, when one start creating an article and then decide to not save it, one is cancelling the action not closing it...

@brianteeman
Copy link
Contributor

the text didnt change as shown in he screenhot and remains as cancel

But "close" would be wrong on this place, when one start creating an article and then decide to not save it, one is cancelling the action not closing it...

I agree but the test instructions show it as changed to Close in the expected behaviour

@chmst
Copy link
Contributor Author

chmst commented Mar 24, 2023

Sorry, was absent.
The testing instructions said "all toolbars" have the exit icon.
I have chosen one Toolbar as an example so that testers see an exit icon.
We have buttons with text 'close" and with text "cancel". I am not sure if this quite consistent, but this is not in scope of this PR. They all had the same "x" icon, now they all have the exit-icon.

@brianteeman
Copy link
Contributor

but your screenshot shows "articles new" with the text "close"
and my screenshot shows "articles new" with the text "cancel"

/me confused

@chmst
Copy link
Contributor Author

chmst commented Mar 24, 2023

My apologies. I was playing with different texts on the buttons and got the wrong screenshot. This is the correct oen for article edit

grafik

On categories it is close

grafik

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 11e73c7


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

@Quy Quy removed UI/UX Small A PR which only has a small change labels Mar 24, 2023
@Quy
Copy link
Contributor

Quy commented Mar 24, 2023

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 24, 2023
@Hackwar Hackwar added the bug label Apr 8, 2023
@obuisard obuisard added this to the Joomla! 4.3.2 milestone May 4, 2023
@obuisard obuisard merged commit 758a71d into joomla:4.3-dev May 4, 2023
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 4, 2023
@obuisard
Copy link
Contributor

obuisard commented May 4, 2023

Thanks Christiane @chmst !

@laoneo
Copy link
Member

laoneo commented May 5, 2023

Now the "Cancel" label in the articles and categories views are shown with "Close". That's why the system tests in 4.4 are failing. Is this intended?
image

@laoneo
Copy link
Member

laoneo commented May 5, 2023

If yes, then such a change should never be merged in a patch release.

obuisard added a commit that referenced this pull request May 5, 2023
@brianteeman
Copy link
Contributor

Sorry @laoneo you are wrong. (If the system tests are failing then it is because the system test is wrong.

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.

@laoneo
Copy link
Member

laoneo commented May 5, 2023

When you click the "New" button on the 4.3-dev branch Then you will see that Close is shown and not cancel. This is different than on 4.3.1.

@brianteeman
Copy link
Contributor

Dev

image

4.3.1

image

@ReLater
Copy link
Contributor

ReLater commented Jan 10, 2024

Will this be ported to J5? Because the question came up in german Joomla forum https://forum.joomla.de/thread/18151-datei-l%C3%B6schen-datei-schliessen/?postID=147003#post147003

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.