Bug 1830004: Add listener for keys in search page #5248

bipuladh · 2020-04-30T16:27:01Z

Scrshots:

Mobile view:

/assign @spadgett

spadgett

Thanks, @bipuladh. This will need a Bugzilla to fix in 4.5.

spadgett · 2020-04-30T16:53:20Z

frontend/public/components/autocomplete.tsx

@@ -83,7 +86,7 @@ const AutocompleteInput: React.FC<AutocompleteInputProps> = (props) => {
          ref={textInputRef}
        />
        <span className="form-control-feedback form-control-feedback--keyboard-hint">
-          <kbd>/</kbd>
+          <kbd>{!isActive ? `/` : 'Esc'}</kbd>


I think this change is a bit more confusing than helpful. We're overloading what the hint means. Previously it always meant focus this element, now it means different things depending on the state. I prefer the previous behavior of just removing the hint on focus.

I notice we're no longer removing the / keyboard hint in other places when the input is focused. That looks like a bug.

Oh okay. I will remove the Esc. And yes we were missing on the hiding part. I realized while writing this.

@bipuladh I added the hiding back in #5252

spadgett · 2020-04-30T16:54:22Z

frontend/public/style/_forms.scss

+.co-query-filter:focus + .form-control-feedback--keyboard-hint {
+  right: 10px;
+}


I'm guessing this change isn't needed if we remove the Esc hint?

spadgett · 2020-04-30T16:56:06Z

frontend/public/components/search-filter-dropdown.tsx

+const textInputKeyHandler = {
+  Escape: KeyEventModes.HIDE,
+  '/': KeyEventModes.FOCUS,
+};


Can we make this a common component instead of repeating it in multiple places? That way we consistently have the same keybindings and behavior.

I will move it to shared.

frontend/packages/console-shared/src/hooks/document-listener.ts

openshift-ci-robot · 2020-04-30T17:25:53Z

@bipuladh: This pull request references Bugzilla bug 1830004, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1830004: Add listener for keys in search page

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-04-30T17:27:56Z

@bipuladh: This pull request references Bugzilla bug 1830004, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1830004: Add listener for keys in search page

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

bipuladh · 2020-04-30T17:28:04Z

/kind bug

bipuladh · 2020-04-30T17:57:54Z

/retest

spadgett

Thanks, nice to see so much code removed switching to a common component 👍

frontend/public/components/factory/list-page.jsx

frontend/packages/console-shared/src/hooks/document-listener.ts

frontend/public/components/RBAC/role.jsx

frontend/public/components/cluster-settings/global-config.tsx

frontend/public/components/events.jsx

frontend/public/components/monitoring.tsx

frontend/public/style/_forms.scss

frontend/public/components/factory/list-page.jsx

frontend/public/style/_forms.scss

spadgett · 2020-05-06T20:44:32Z

@bipuladh Looks like you introduced a test failure.

spadgett · 2020-05-06T20:46:11Z

frontend/__tests__/components/factory/list-page.spec.tsx

-    expect(input.props().onChange).toEqual(onChange);
+    expect(input.props().onChange()).toEqual(((e) => onChange(e))());


Is this change still needed? I thought we went back to onChange={onChange}.

Not anymore.

spadgett

/approve
/lgtm

openshift-ci-robot · 2020-05-06T21:15:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bipuladh, spadgett

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

spadgett · 2020-05-06T21:15:39Z

Thanks @bipuladh, looks good 👍

openshift-bot · 2020-05-06T21:21:44Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-06T21:34:46Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-05-06T22:26:44Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-05-07T01:44:03Z

@bipuladh: All pull requests linked via external trackers have merged: openshift/console#5248. Bugzilla bug 1830004 has been moved to the MODIFIED state.

In response to this:

Bug 1830004: Add listener for keys in search page

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot assigned spadgett Apr 30, 2020

openshift-ci-robot requested review from rhamilto and sg00dwin April 30, 2020 16:27

openshift-ci-robot added the component/shared Related to console-shared label Apr 30, 2020

spadgett reviewed Apr 30, 2020

View reviewed changes

bipuladh force-pushed the search-listener branch from 401c3f2 to 23085cb Compare April 30, 2020 17:24

bipuladh changed the title ~~Add listener for keys in search page~~ Bug 1830004: Add listener for keys in search page Apr 30, 2020

openshift-ci-robot added the bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. label Apr 30, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 30, 2020

bipuladh requested a review from spadgett April 30, 2020 17:26

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 30, 2020

bipuladh force-pushed the search-listener branch from 23085cb to 6849655 Compare May 4, 2020 13:05

openshift-ci-robot added component/dev-console Related to dev-console component/monitoring Related to monitoring labels May 4, 2020

bipuladh force-pushed the search-listener branch 3 times, most recently from f4347c7 to 3b5226e Compare May 4, 2020 14:17

spadgett reviewed May 4, 2020

View reviewed changes

bipuladh force-pushed the search-listener branch from 3b5226e to 870e7d2 Compare May 4, 2020 21:30

bipuladh requested a review from spadgett May 4, 2020 21:37

spadgett reviewed May 5, 2020

View reviewed changes

frontend/public/components/factory/list-page.jsx Outdated Show resolved Hide resolved

frontend/public/style/_forms.scss Outdated Show resolved Hide resolved

bipuladh force-pushed the search-listener branch 2 times, most recently from c0f4e17 to 8d38e86 Compare May 5, 2020 16:22

bipuladh requested a review from spadgett May 5, 2020 16:28

spadgett reviewed May 6, 2020

View reviewed changes

Unified input component for filters console wide

155e6ab

bipuladh force-pushed the search-listener branch from 8d38e86 to 155e6ab Compare May 6, 2020 21:12

spadgett approved these changes May 6, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 6, 2020

spadgett added this to the v4.5 milestone May 6, 2020

openshift-merge-robot merged commit e94e491 into openshift:master May 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1830004: Add listener for keys in search page #5248

Bug 1830004: Add listener for keys in search page #5248

bipuladh commented Apr 30, 2020 •

edited

spadgett left a comment

spadgett Apr 30, 2020

spadgett Apr 30, 2020

bipuladh Apr 30, 2020 •

edited

spadgett Apr 30, 2020

spadgett Apr 30, 2020

bipuladh Apr 30, 2020

spadgett Apr 30, 2020

bipuladh Apr 30, 2020

openshift-ci-robot commented Apr 30, 2020

openshift-ci-robot commented Apr 30, 2020

bipuladh commented Apr 30, 2020

bipuladh commented Apr 30, 2020

spadgett left a comment

spadgett commented May 6, 2020

spadgett May 6, 2020

bipuladh May 6, 2020

spadgett left a comment

openshift-ci-robot commented May 6, 2020

spadgett commented May 6, 2020

openshift-bot commented May 6, 2020

openshift-bot commented May 6, 2020

openshift-bot commented May 6, 2020

openshift-ci-robot commented May 7, 2020

		expect(input.props().onChange).toEqual(onChange);
		expect(input.props().onChange()).toEqual(((e) => onChange(e))());

Bug 1830004: Add listener for keys in search page #5248

Bug 1830004: Add listener for keys in search page #5248

Conversation

bipuladh commented Apr 30, 2020 • edited

spadgett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bipuladh Apr 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Apr 30, 2020

openshift-ci-robot commented Apr 30, 2020

bipuladh commented Apr 30, 2020

bipuladh commented Apr 30, 2020

spadgett left a comment

Choose a reason for hiding this comment

spadgett commented May 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented May 6, 2020

spadgett commented May 6, 2020

openshift-bot commented May 6, 2020

openshift-bot commented May 6, 2020

openshift-bot commented May 6, 2020

openshift-ci-robot commented May 7, 2020

bipuladh commented Apr 30, 2020 •

edited

bipuladh Apr 30, 2020 •

edited