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

feat(ui): Add command-k icons to search bar #9194

Merged

Conversation

jjoyce0510
Copy link
Collaborator

Summary

Previously, you could auto-select search using command k but this feature was hidden. In this PR we add iconography to search bar to display that you can select the bar using command k.

We also fix a minor issue where on some search bars command k was showing when it should not have.

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions github-actions bot added the product PR or Issue related to the DataHub UI/UX label Nov 6, 2023
Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

cool beans definitely love the command K situation. idk if we want to always show it in the search bar - I don't have too strong of an opinion and could be convinced - but maybe we don't want to busy up the search bar everywhere for an optional delight feature?

one option that notion does is shows the shortcut on hover. maybe that's an option?

@@ -411,6 +421,7 @@ export const SearchBar = ({
</>
}
ref={searchInputRef}
suffix={(showCommandK && !isFocused && <CommandK />) || null}
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you think we need this command k icon? tbh it feels a little busy and maybe not necessary to show for this. we could show something as a tooltip when you hover over the search bar that it's possible to do command K?

Comment on lines +304 to +318
if (showCommandK) {
const handleKeyDown = (event) => {
// Support command-k to select the search bar.
// 75 is the keyCode for 'k'
if ((event.metaKey || event.ctrlKey) && event.keyCode === 75) {
(searchInputRef?.current as any)?.focus();
}
};
document.addEventListener('keydown', handleKeyDown);
return () => {
document.removeEventListener('keydown', handleKeyDown);
};
}
return () => null;
}, [showCommandK]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh nice so we already supported this!

Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

good stuff

@jjoyce0510 jjoyce0510 merged commit 4577001 into datahub-project:master Nov 7, 2023
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR or Issue related to the DataHub UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants