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.

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

Board switcher keyboard shortcut navigation #3242

Merged
merged 16 commits into from
Jul 28, 2022
Merged

Board switcher keyboard shortcut navigation #3242

merged 16 commits into from
Jul 28, 2022

Conversation

Pinjasaur
Copy link
Contributor

Summary

Implements esc/up/down/enter keyboard shortcuts for better navigating the Boards switcher—similar to Channels.

Ticket Link

closes #2755

Screen.Recording.2022-06-16.at.1.29.54.PM.mov

Pinjasaur added 2 commits June 16, 2022 13:18
@Pinjasaur
feat: ESC closes quick switch dialog
f98a958 
Similar to Cmd/Ctrl + K, need to do it via native event listener when
focused on an <input>.
@Pinjasaur
feat: implement up/down/enter shortcuts
f877a6e
@Pinjasaur Pinjasaur requested a review from a team as a code owner June 16, 2022 18:37
@Pinjasaur Pinjasaur requested review from Rajat-Dabade, chenilim and asaadmahmood and removed request for a team June 16, 2022 18:37
@Pinjasaur Pinjasaur added 1: UX Review Requires review by ux 2: Dev Review Requires review by a core committer labels Jun 16, 2022
@sbishel sbishel requested review from jespino and removed request for chenilim June 17, 2022 17:48
jespino
jespino reviewed Jun 20, 2022
View reviewed changes
webapp/src/components/boardsSwitcherDialog/boardSwitcherDialog.tsx Outdated
@@ -81,6 +85,7 @@ const BoardSwitcherDialog = (props: Props): JSX.Element => {
title={title}
subTitle={subTitle}
searchHandler={searchHandler}
refs={refs}
Copy link
Contributor

Choose a reason for hiding this comment

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

All this refs passings looks a bit convoluted to me. Couldn't it be something like an state at this level with the selected id? and an onSelect callback or something like that?

I always find passing refs down the components really hard to track.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, it wasn't as straightforward as I was expecting it to be. My reasoning for using refs was it let me use the native .focus() and .click() methods on the underlying native elements.

Since the parent component (this) defines the search handler which returns the JSX components and passes that definition into the child component which calls it and does the rendering, it seemed to make sense to define the refs in the parent so the child could use the native methods I mentioned above. onSelect did come to mind, but it's not available for all elements, only <input> and <textarea>.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, what I meant is. move the selected state to the BoardSwitcherDialog component, update the selected state using a callback that is passed to the SearchDialog component, and then, you can use a useEffect call to update the focus on every selected change. The syntethic click, is not going to be needed, because you are already there, and you have access to the selectBoard function. You are going to still need the refs, but you are not passing them around that is what makes it more confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jespino Refactored it as requested and while I agree passing refs as props is confusing, I'm not positive trading passing the selected state is less confusing. Additionally, needed to move the keyboard event listeners around. I believe the functionality is the same, but I still prefer my original approach after doing the refactor.

Let me know your thoughts—take care not to merge as the cherrypick label(s) will need to be updated.

jespino
jespino requested changes Jun 21, 2022
View reviewed changes
Copy link
Contributor

@jespino jespino left a comment

Choose a reason for hiding this comment

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

I have some doubts about the approach.

@Pinjasaur Pinjasaur requested a review from jespino June 30, 2022 20:43
@Pinjasaur
Copy link
Contributor Author

/update-branch

@Pinjasaur
Copy link
Contributor Author

@jespino friendly ping to re-review when you have time 🙂

@Pinjasaur
Copy link
Contributor Author

/update-branch

@Pinjasaur
Copy link
Contributor Author

/update-branch

@Pinjasaur
Copy link
Contributor Author

/update-branch

@Pinjasaur Pinjasaur added this to the v7.2 milestone Jul 19, 2022
@Pinjasaur
Copy link
Contributor Author

/update-branch

@Pinjasaur Pinjasaur added the CherryPick/Candidate A candidate for a quality or patch release, but not yet approved label Jul 19, 2022
@Pinjasaur Pinjasaur added CherryPick/Approved Meant for the quality or patch release tracked in the milestone and removed CherryPick/Candidate A candidate for a quality or patch release, but not yet approved labels Jul 22, 2022
@Pinjasaur
Copy link
Contributor Author

/update-branch

@sbishel
Copy link
Collaborator

sbishel commented Jul 27, 2022

@jespino - Would you review this again?

jespino
jespino approved these changes Jul 27, 2022
View reviewed changes
Copy link
Contributor

@jespino jespino left a comment

Choose a reason for hiding this comment

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

It still looks to me a bit complicated code, but at least no passing a list of refs makes it more contained. LGTM

@jespino
Copy link
Contributor

jespino commented Jul 27, 2022

/update-branch

harshilsharma63
harshilsharma63 approved these changes Jul 28, 2022
View reviewed changes
Copy link
Member

@harshilsharma63 harshilsharma63 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Pinjasaur Pinjasaur merged commit f241fc1 into main Jul 28, 2022
@mattermod
Copy link
Contributor

Cherry pick is scheduled.

@Pinjasaur Pinjasaur deleted the gh-2755-board-switcher branch July 28, 2022 17:10
mattermost-build pushed a commit to mattermost-build/focalboard that referenced this pull request Jul 28, 2022
@Pinjasaur @mattermost-build
Board switcher keyboard shortcut navigation (mattermost-community#3242)
9e3ee34 
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
(cherry picked from commit f241fc1)
@mattermost-build mattermost-build mentioned this pull request Jul 28, 2022
Merged
@mattermod mattermod added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Jul 28, 2022
Pinjasaur added a commit that referenced this pull request Jul 28, 2022
@mattermost-build @mattermod @Pinjasaur
Automated cherry pick of #3242 (#3456)
3614706 
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
Co-authored-by: Paul Esch-Laurent <paul.esch-laurent@mattermost.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harshilsharma63 harshilsharma63 harshilsharma63 approved these changes

@asaadmahmood asaadmahmood asaadmahmood approved these changes

@jespino jespino jespino approved these changes

@Rajat-Dabade Rajat-Dabade Awaiting requested review from Rajat-Dabade Rajat-Dabade was automatically assigned from mattermost/core-focalboard

Assignees
No one assigned
Labels
1: UX Review Requires review by ux 2: Dev Review Requires review by a core committer CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone
Projects
None yet
Milestone
v7.2
Development

Successfully merging this pull request may close these issues.

Permissions Branch: Board switcher keyboard nav not working
6 participants
@Pinjasaur @sbishel @jespino @mattermod @asaadmahmood @harshilsharma63