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.

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

fix: adding a query parameter in Playground requires one extra click to type in search field #4295

Merged
merged 5 commits into from Apr 4, 2022

Conversation

MrFlashAccount
Copy link
Contributor

@MrFlashAccount MrFlashAccount commented Mar 31, 2022

Check List

  • Tests has been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Description of Changes Made (if issue reference is not provided)

this PR add focusing in input on opening the menu and on typing within the list

@MrFlashAccount MrFlashAccount requested a review from a team as a code owner March 31, 2022 16:21
@github-actions github-actions bot added the pr:community Contribution from Cube.js community members. label Mar 31, 2022
@MrFlashAccount MrFlashAccount changed the title fix: focus on input on open popup in playground and focus on input on… fix: adding a query parameter in Playground requires one extra click to type in search field Mar 31, 2022
@codecov
Copy link

codecov bot commented Mar 31, 2022

Codecov Report

Merging #4295 (d0cdd77) into master (add2dd3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master    #4295    +/-   ##
========================================
  Coverage   59.02%   59.02%            
========================================
  Files         133      133            
  Lines       10885    10885            
  Branches     2565     2662    +97     
========================================
  Hits         6425     6425            
+ Misses       4185     3987   -198     
- Partials      275      473   +198     
Flag Coverage Δ
cube-backend 59.02% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/cubejs-api-gateway/src/jwk.ts 11.47% <0.00%> (ø)
packages/cubejs-backend-shared/src/env.ts 33.33% <0.00%> (ø)
packages/cubejs-api-gateway/src/gateway.ts 66.05% <0.00%> (ø)
packages/cubejs-api-gateway/src/graphql.ts 4.16% <0.00%> (ø)
packages/cubejs-backend-shared/src/time.ts 28.57% <0.00%> (ø)
packages/cubejs-backend-shared/src/proxy.ts 23.52% <0.00%> (ø)
packages/cubejs-backend-shared/src/track.ts 29.72% <0.00%> (ø)
packages/cubejs-backend-shared/src/errors.ts 28.57% <0.00%> (ø)
packages/cubejs-api-gateway/src/sql-server.ts 6.89% <0.00%> (ø)
packages/cubejs-backend-shared/src/helpers.ts 56.75% <0.00%> (ø)
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update add2dd3...d0cdd77. Read the comment docs.

@tenphi tenphi added enhancement New feature proposal and removed pr:community Contribution from Cube.js community members. labels Mar 31, 2022
overlay={
<Menu
onKeyDown={(e) => {
if (['ArrowDown', 'ArrowUp'].includes(e.key)) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks weird. We do focus on every key except these two? It should break keyboard navigation hard.

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'm not sure it's weird, though. When we navigate the menu and can't find the needed item, we can start typing, and even if the input isn't focused, the text will be typed in the input. This is a nice pattern :)

Screen.Recording.2022-03-31.at.20.56.21.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, there is no keyboard navigation except navigation within the list. I've tested.

Copy link
Member

@tenphi tenphi Mar 31, 2022

Choose a reason for hiding this comment

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

The pattern is good. But keyboard navigation includes: Space, Enter, PageUp, PageDown, Up, Down, Right, Left (yeah, horizontal, even in a vertical layout). Maybe we should test an event for a character and only in that case do the focus.

@tenphi tenphi merged commit 4abab96 into cube-js:master Apr 4, 2022
@MrFlashAccount MrFlashAccount deleted the bugfix/CC-266 branch April 4, 2022 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants