LBRYCommunity/lbry-desktop

Fork

You've already forked lbry-desktop

Code Issues 596 Pull requests 6 Packages Projects Releases 238 Wiki Activity

Fix MenuButton behavior from toggle dropdown onMousePress to onClick #7335

Merged

kornatskyi merged 5 commits from master into master

2021-12-10 00:05:37 +01:00

Conversation 4 Commits 5 Files changed 1 +19

kornatskyi commented

2021-12-09 06:51:14 +01:00

(Migrated from github.com)

Copy link

Fixes

Issue Number: #7326

What is the current behavior?

MenuButton in the component claimMenuList toggles MenuList(dropdown) on mosePress event

What is the new behavior?

MenuButton in the component claimMenuList toggles MenuList(dropdown) on click event

Other information

MenuButtons in other components still toggle dropdowns on mouse press. It doesn't relate to this bugfix but my proposition is to change their behavior to toggle dropdown on click as well.

PR Checklist

Toggle...

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting)
Refactoring (no functional changes)
Documentation changes
Other - Please describe:

Please check all that apply to this PR using "x":

I have checked that this PR is not a duplicate of an existing PR (open, closed or merged)
I added a line describing my change to CHANGELOG.md
I have checked that this PR does not introduce a breaking change
This PR introduces breaking changes and I have provided a detailed explanation below

## Fixes Issue Number: #7326  ## What is the current behavior? `MenuButton` in the component `claimMenuList` toggles `MenuList`(dropdown) on mosePress event ## What is the new behavior? `MenuButton` in the component `claimMenuList` toggles `MenuList`(dropdown) on click event ## Other information  `MenuButton`s in other components still toggle dropdowns on mouse press. It doesn't relate to this bugfix but my proposition is to change their behavior to toggle dropdown on click as well. ## PR Checklist  <details><summary>Toggle...</summary> What kind of change does this PR introduce? - [x] Bugfix - [ ] Feature - [ ] Code style update (formatting) - [ ] Refactoring (no functional changes) - [ ] Documentation changes - [ ] Other - Please describe: Please check all that apply to this PR using "x": - [x] I have checked that this PR is not a duplicate of an existing PR (open, closed or merged) - [ ] I added a line describing my change to CHANGELOG.md - [x] I have checked that this PR does not introduce a breaking change - [ ] This PR introduces breaking changes and I have provided a detailed explanation below </details>

jessopb commented

2021-12-09 16:52:11 +01:00

(Migrated from github.com)

Copy link

Thanks for the pr - I tried it and it indeed activated the menu onClick rather than on mouse down.
Two questions -

does this have any implications for accessibility?
should we try upgrading to v16 of reach-ui? There's a change there that might be relevant.
https://github.com/reach/reach-ui/releases/tag/v0.16.0

Thanks for the pr - I tried it and it indeed activated the menu onClick rather than on mouse down. Two questions - 1) does this have any implications for accessibility? 2) should we try upgrading to v16 of reach-ui? There's a change there that might be relevant. https://github.com/reach/reach-ui/releases/tag/v0.16.0

kornatskyi commented

2021-12-09 18:46:03 +01:00

(Migrated from github.com)

Copy link

Thank you for your review.

Yes, I checked and it has an issue with handling the Enter key when an element is focused. I will fix it.
I took a look at the reach-ui v0.16.0 code, and basically what they did is add a delay (400 ms) between mouse press and enabling selection on the dropdown. So it kind of solves the issue if a user doesn't click faster than 400ms. And it also will select a section if after pressing the mouse button you move the cursor 8px. I've tried their new package, and it solves the problem a little bit but not completely.

Thank you for your review. 1. Yes, I checked and it has an issue with handling the `Enter` key when an element is focused. I will fix it. 2. I took a look at the reach-ui v0.16.0 code, and basically what they did is add a delay (400 ms) between mouse press and enabling selection on the dropdown. So it kind of solves the issue if a user doesn't click faster than 400ms. And it also will select a section if after pressing the mouse button you move the cursor 8px. I've tried their new package, and it solves the problem a little bit but not completely.

👍 1

jessopb commented

2021-12-09 20:26:42 +01:00

(Migrated from github.com)

Copy link

Thanks. Let me know when it's ready.

kornatskyi commented

2021-12-09 23:12:24 +01:00

(Migrated from github.com)

Copy link

Hey, I've messed up a little bit with git, sorry for those extra commits. I'm not sure if I have to do something else, or you already can accept my changes?

jessopb (Migrated from github.com) approved these changes 2021-12-09 23:23:46 +01:00

Reviewers

No reviewers

jessopb

Labels

Clear labels

Work is part of a proposal

Beamer is waiting on you!

Discuss this issue at the next planning meeting, then remove this label

Welcome to Hacktoberfest

No knowledge of the existing code required

level: 2

Some knowledge of the existing code is recommended

level: 3

Significant knowledge of the existing code is recommended

level: 4

Intimate knowledge of the existing code is recommended

merge when green

needs: exploration

Solution unclear, needs research

needs: grooming

Issue needs to be groomed before work can start

needs: priority

Priority level needs to be defined

needs: repro

Issue needs step-by-step instructions on how to reproduce in latest code

needs: tech design

Needs technical design signoff before implementation can begin

Issue is blocking release, do ASAP

priority: high

Work needs to be moved into sprint ASAP

priority: low

Work should be done but can stay in the backlog for now

priority: medium

Work needs to be done within 2-3 sprints

protocol dependent

recsys

redesign

general redesign not prioritize for anyone release

regression

resilience

sdk dependent

Requires work on lbry-sdk repo

Tom's Wishlist

trending

type: bug

Existing functionality is wrong or broken

type: discussion

A conversation. No specific changes requested

type: improvement

Existing (or partially existing) functionality needs to be changed

type: new feature

New functionality that does not exist yet

type: refactor

Minimal user-visible changes, but significant internal work

type: task

Either work that's not related to the code, or a small chore that does not fit into other categories

type: testing

Solution needs additional user testing

unplanned

Work that was not planned into the spirnt, took priority over planned work

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbry-desktop#7335

Reference in a new issue

Repository

LBRYCommunity/lbry-desktop

Title

Body

No description provided.

Delete branch "master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?