-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
feat: move keyboard shortcut hints to tooltips #12100
feat: move keyboard shortcut hints to tooltips #12100
Conversation
e2dbaae
to
15be611
Compare
Codecov Report
@@ Coverage Diff @@
## feat-sql-toolbar #12100 +/- ##
====================================================
- Coverage 67.32% 63.51% -3.81%
====================================================
Files 963 963
Lines 47390 47381 -9
Branches 4620 4620
====================================================
- Hits 31903 30092 -1811
- Misses 15374 17104 +1730
- Partials 113 185 +72
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Do you want me to merge it?
<Button {...commonBtnProps} cta onClick={stopQuery}> | ||
<Button | ||
{...commonBtnProps} | ||
cta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's cta here? i think we can remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like the cta
prop makes it all caps.
@@ -67,7 +72,7 @@ const RunQueryActionButton = ({ | |||
cta | |||
onClick={() => runQuery(true)} | |||
key="run-async-btn" | |||
tooltip={t('Run query asynchronously (Ctrl + ↵)')} | |||
tooltip={t('Run query (Ctrl + Return)')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tooltip={t('Run query (Ctrl + Return)')} | |
tooltip={t('Run Query (Ctrl + Return)')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The design has Query
as lowercase, but let's check with Stephen!
SUMMARY
Updated Keyboard Shortcuts Interface
Remove existing keyboard shortcuts button
Hover tooltips should show up on the following:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TEST PLAN
Jest tests
ADDITIONAL INFORMATION