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

[4.0] Versions Toolbar when button disabled #35761

Merged
merged 4 commits into from
Nov 28, 2021

Conversation

rjharishabh
Copy link
Contributor

@rjharishabh rjharishabh commented Oct 5, 2021

Pull Request for Issue #35377.

Summary of Changes

Add if to check and respond only when the button is not disabled

Testing Instructions

please see issue

Don't forget to build JS npm run build:js

Actual result BEFORE applying this Pull Request

Disabled buttons respond

Expected result AFTER applying this Pull Request

Disabled buttons do not respond

preview.mp4

Documentation Changes Required

No

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Oct 5, 2021
@brianteeman
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on b632dce

obviously incorrect


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

@rjharishabh
Copy link
Contributor Author

rjharishabh commented Oct 5, 2021

Hi Brian, is it not working or any other issue

@brianteeman
Copy link
Contributor

Try it and you will see

@brianteeman
Copy link
Contributor

still broken

@brianteeman
Copy link
Contributor

I see that I have to be more explicit - I was hoping that you would see at least one of the errors yourself.

  1. In this PR you are reverting a recent change in the language string. As a result if you actually try this code on the current repository then the messages are not translated.

Example

image

  1. As you have correctly disabled the ability to click on a disabled button then the condition that would cause the message to select one version can never be met. If that was intended then the code can be greatly simplified.

@rjharishabh
Copy link
Contributor Author

  1. In this PR you are reverting a recent change in the language string. As a result, if you actually try this code on the current repository then the messages are not translated.

Updated Thank you
Actually, I have not pulled latest changes from Github 😂

@rjharishabh
Copy link
Contributor Author

  1. As you have correctly disabled the ability to click on a disabled button then the condition that would cause the message to select one version can never be met. If that was intended then the code can be greatly simplified.

When we select more than one version to restore or preview, then this message will help Please select one version.
Similarly, when we select one or three instead of two to compare, alert will be displayed Please select two versions.

@jwaisner
Copy link
Member

@rjharishabh , I tested and now the disabled buttons no longer do anything but the error you got previously regarding "Please select one version" does not appear. Also the disabled icon does not seem to show by the cursor at all.


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

@rjharishabh
Copy link
Contributor Author

but the error you got previously regarding "Please select one version" does not appear.

Select two versions to preview, then this message will be displayed.

@rjharishabh
Copy link
Contributor Author

I've updated the description with a video.

@richard67
Copy link
Member

@rjharishabh The description in issue #35377 mentioned 2 things in expected result:

  • If a button is disabled - clicking ANYWHERE on that button should be disabled.
  • If a button is disabled - hovering ANYWHERE on that button should show a disabled cursor
    But your PR seems to implement only the first one. At least I can't see any pointer change when hovering the buttons. Is that by purpose? Or is that something browser-specific not to be solved with a PR?

@rjharishabh
Copy link
Contributor Author

I have not done anything here for the disabled cursor
In the issue, the disabled icon is shown in Phil's video and with safari on mac
It may be a browser-specific thing, but I am not sure

@richard67
Copy link
Member

I have not done anything here for the disabled cursor In the issue, the disabled icon is shown in Phil's video and with safari on mac It may be a browser-specific thing, but I am not sure

@rjharishabh Firefox, Chrome and Edge do not do that, so maybe it is a Safari thing or @PhilETaylor wanted to have some css for that purpose. But for me this PR here is sufficient as it is, because the buttons are properly disabled in HTML when I inspect them, and the pointer does not change like it would when there was an enabled button or link, so it is clear that these buttons are disabled.

@PhilETaylor Please report back if you did expect more to be done for solving your issue. Thanks in advance.

@richard67
Copy link
Member

I have tested this item ✅ successfully on 342831d

The buttons are properly disabled now when nothing is selected.

If a wrong number of versions is selected, i.e. one version or more than 2 versions for the Compare button or 2 or more versions for the Preview button, the right error message is still show like it was before.


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

@RickR2H
Copy link
Member

RickR2H commented Oct 24, 2021

I have tested this item ✅ successfully on 342831d

Bug confirmed and patch is working!


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

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 24, 2021
@wilsonge wilsonge merged commit ca807c7 into joomla:4.0-dev Nov 28, 2021
@wilsonge
Copy link
Contributor

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 28, 2021
@wilsonge wilsonge added this to the Joomla 4.0.5 milestone Nov 28, 2021
@rjharishabh rjharishabh deleted the preview branch April 18, 2022 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants