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

Auto-focus on the Field input when clicking Add filter #355

Merged
merged 2 commits into from
May 24, 2021
Merged

Auto-focus on the Field input when clicking Add filter #355

merged 2 commits into from
May 24, 2021

Conversation

galangel
Copy link
Contributor

Signed-off-by: galangel gal0angel@gmail.com

Description

Added Auto-focus on the Field input when clicking Add filter
Done by setting the initial focus of the popover with the relevant selector

example:
autofocus

Issues Resolved

Add filter form initial focus should be the Field input

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

@galangel
correct autofocus
0ccc3c1 
Signed-off-by: galangel <gal0angel@gmail.com>
@galangel galangel changed the title correct autofocus Add Auto-focus on the Field input when clicking Add filter May 13, 2021
@galangel galangel changed the title Add Auto-focus on the Field input when clicking Add filter Auto-focus on the Field input when clicking Add filter May 13, 2021
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 0ccc3c1

mihirsoni
mihirsoni reviewed May 13, 2021
View reviewed changes
Copy link
Contributor

@mihirsoni mihirsoni left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @galangel This is really good, had one small question. LGTM otherwise.

src/plugins/data/public/ui/filter_bar/filter_bar.tsx Outdated
@@ -121,7 +121,7 @@ function FilterBarUI(props: Props) {
withTitle
panelPaddingSize="none"
ownFocus={true}
initialFocus=".filterEditor__hiddenItem"
initialFocus='[data-test-subj="filterFieldSuggestionList"] [data-test-subj="comboBoxSearchInput"]'
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't data-test-subj are meant for functional tests ? Can we provide another class ?

Copy link
Contributor Author

@galangel galangel May 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hi, you suggest switching to a class? sure, I wouldn't mind

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be class or a unique id attribute, I am fine either way as long as this will take care It should be good.

@seanneumann
Copy link
Contributor

I like this. Team is reviewing.

@kgcreative
Copy link
Member

+1 - This is a nice enhancement, particularly for keyboard nav users.

@galangel
use class
779e1f3 
Signed-off-by: galangel <gal0angel@gmail.com>
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 779e1f3

mihirsoni
mihirsoni approved these changes May 18, 2021
View reviewed changes
Copy link
Contributor

@mihirsoni mihirsoni left a comment

Choose a reason for hiding this comment

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

LGTM !! Thanks for the changes @galangel

@JacobBrandt JacobBrandt mentioned this pull request May 18, 2021
Merged
5 tasks
@kavilla
Copy link
Member

kavilla commented May 20, 2021

I believe @JacobBrandt made a good catch but seeing you will revert that change in the other PR I will verify this PR tonight.

@kavilla
Copy link
Member

kavilla commented May 24, 2021

Tests all passing (func included), everything looks good to me. Approving and merging in.

Thank you so much.

@kavilla kavilla linked an issue May 24, 2021 that may be closed by this pull request
Add filter form initial focus should be the Field input [BUG] #354
Closed
@kavilla kavilla self-requested a review May 24, 2021 07:00
@kavilla kavilla added Beta bug Something isn't working labels May 24, 2021
@kavilla kavilla merged commit 4a47d8c into opensearch-project:main May 24, 2021
kavilla pushed a commit that referenced this pull request May 25, 2021
@galangel @kavilla
Auto-focus on the Field input when clicking Add filter (#355)
03e7020 
Added Auto-focus on Add Filter Input

Done by setting the initial focus of the popover with the relevant selector

* correct autofocus

Signed-off-by: galangel <gal0angel@gmail.com>

* use class

Signed-off-by: galangel <gal0angel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kavilla kavilla kavilla approved these changes

@mihirsoni mihirsoni mihirsoni approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add filter form initial focus should be the Field input [BUG]
6 participants
@galangel @opensearch-ci-bot @seanneumann @kgcreative @kavilla @mihirsoni