Skip to content

Fix 22085 #23166

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 19, 2019
Merged

Fix 22085 #23166

merged 1 commit into from
Jul 19, 2019

Conversation

geet07
Copy link

@geet07 geet07 commented Jun 7, 2019

Description (*)

Fixed Issues (if relevant)

  1. magento/magento2#<22085>: Not focusable moreButton on Catalog page using Tab

Manual testing scenarios (*)

  1. You need to have a product with more than 3 swatches option
    Option in
    Stores -> Configuration -> Catalog / Catalog -> Swatches per Product set to 3
    Show Swatches in Product List set to Yes
  2. In the products list hover on more link, now it is focusable

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Jun 7, 2019

Hi @geet07. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@davidverholen davidverholen self-assigned this Jun 7, 2019
@davidverholen
Copy link
Member

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @davidverholen. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @davidverholen, here is your new Magento instance.
Admin access: https://pr-23166.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@magento-engcom-team
Copy link
Contributor

Hi @davidverholen, thank you for the review.
ENGCOM-5231 has been created to process this Pull Request

@soleksii soleksii self-assigned this Jun 7, 2019
@ysapiga ysapiga self-assigned this Jun 7, 2019
@soleksii
Copy link

soleksii commented Jun 7, 2019

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @stoleksiy. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @stoleksiy, here is your new Magento instance.
Admin access: https://pr-23166.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@soleksii
Copy link

soleksii commented Jun 7, 2019

Hi @geet07 ! According to the magento documentation, the styles for the links should be underlined (see here).
Should be like this:

screen

With PR changes actual result is:

screen2

@VladimirZaets Is this the expected result?

Thanks!

orlangur
orlangur requested changes Jun 7, 2019
View reviewed changes
Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash changes into a single commit so that we have perfectly clean history 😉

Also, no unrelated tickets must be present in commit history in the first place.

@ghost ghost assigned orlangur Jun 7, 2019
@geeta07 geeta07 force-pushed the fix-22085 branch 2 times, most recently from 2dc1805 to fe27655 Compare June 12, 2019 11:10
@geeta07 geeta07 deleted the fix-22085 branch June 12, 2019 13:00
@geet07 geet07 closed this Jun 13, 2019
@m2-assistant
Copy link

m2-assistant bot commented Jun 13, 2019

Hi @geet07, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@geet07 geet07 mentioned this pull request Jun 13, 2019
Closed
4 tasks
@geet07
Copy link
Author

geet07 commented Jun 20, 2019 •
edited
Loading

Hi, @geet07. I think the problem is still not fixed
according to @stoleksiy comment. Actual result:
te
Could you take a look?

Hi @engcom-Delta, @stoleksiy
Thank for your message.

I have checked with this, but More button is not a link, it is not containing any URL for the redirect. it's using for the only javascript. So as my understanding it is correct. Please let me know if you have any concern here.

@geet07
Copy link
Author

geet07 commented Jun 21, 2019

Hi @engcom-Delta, @engcom-Golf :
Can you please test this issue. And also check my last comment and let me know if I am wrong here.

orlangur
orlangur requested changes Jun 22, 2019
View reviewed changes
Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geet07, as per Slack discussion

https://devdocs.magento.com/guides/v2.3/pattern-library/controls/buttons/buttons.html

When Not to Use
Do not use buttons to indicate a link to more information or to a task not related to primary flow.

I believe we should treat “More” as a link here and thus make it underlined.

After changes are applied and all builds are green, please squash them into a single commit so that we have perfectly clean history 😉

@geeta07 geeta07 force-pushed the fix-22085 branch 2 times, most recently from f925997 to 6087e4f Compare June 22, 2019 15:20
@geet07
Copy link
Author

geet07 commented Jun 22, 2019

@geet07, as per Slack discussion

https://devdocs.magento.com/guides/v2.3/pattern-library/controls/buttons/buttons.html

When Not to Use
Do not use buttons to indicate a link to more information or to a task not related to primary flow.

I believe we should treat “More” as a link here and thus make it underlined.

After changes are applied and all builds are green, please squash them into a single commit so that we have perfectly clean history

Hi @orlangur,

Finally I have done git squash :-) , Can you please review and let me know if there is an issue or any changes required

@geet07
Copy link
Author

geet07 commented Jun 25, 2019

Hi @orlangur

Can you please review the change request.

@geet07 geet07 requested a review from orlangur June 26, 2019 06:00
@geet07 geet07 requested a review from davidverholen July 8, 2019 10:09
@geet07
Copy link
Author

geet07 commented Jul 12, 2019 •
edited
Loading

Hi @orlangur,

As per you requested, can you please review the changes

@magento-engcom-team magento-engcom-team added Partner: Ranosys Technologies partners-contribution Pull Request is created by Magento Partner labels Jul 15, 2019
@magento-engcom-team
Copy link
Contributor

Hi @orlangur, thank you for the review.
ENGCOM-5313 has been created to process this Pull Request

@engcom-Delta
Copy link
Contributor

✔️ QA passed

@engcom-Foxtrot engcom-Foxtrot self-assigned this Jul 15, 2019
@magento-engcom-team magento-engcom-team merged commit 2230205 into magento:2.3-develop Jul 19, 2019
@m2-assistant
Copy link

m2-assistant bot commented Jul 19, 2019

Hi @geet07, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.3 milestone Jul 19, 2019
magento-engcom-team pushed a commit that referenced this pull request Jul 19, 2019
@VladimirZaets
ENGCOM-5313: Fix 22085 #23166
6b72f2f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orlangur orlangur orlangur approved these changes

@davidverholen davidverholen Awaiting requested review from davidverholen

Assignees

@orlangur orlangur

@engcom-Delta engcom-Delta

@engcom-Foxtrot engcom-Foxtrot

@engcom-Golf engcom-Golf

Labels
Area: Design/Frontend Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Partner: Ranosys Technologies partners-contribution Pull Request is created by Magento Partner Progress: accept Release Line: 2.3
Projects
None yet
Milestone
Release: 2.3.3
Development

Successfully merging this pull request may close these issues.
10 participants
@geet07 @davidverholen @magento-engcom-team @soleksii @engcom-Delta @orlangur @ysapiga @engcom-Foxtrot @engcom-Golf @geeta07