Skip to content

feat: add kbd hint #1495

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

Venki1402
Copy link

@Venki1402 Venki1402 commented Aug 11, 2025

Screenshot 2025-08-11 at 11 51 07 AM

Copy link
Member

@adamant-pwn adamant-pwn left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the pull request! It generally looks good, but I'd like to clarify some things first (see comments).

}

/* Only show on wider viewports where the inline search is visible */
@media (max-width: 959px) {
Copy link
Member

Choose a reason for hiding this comment

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

Where is this value defined? Can we somehow take it from the theme named constants?

Copy link
Author

Choose a reason for hiding this comment

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

not sure regd. that, can we take 60em instead?

Comment on lines +88 to +96
justify-content: center;
height: 20px;
padding: 0 6px;
border-radius: 4px;
border: 1px solid var(--md-default-fg-color--lightest, rgba(0,0,0,.2));
background: var(--md-default-bg-color, #fff);
color: var(--md-default-fg-color, rgba(0,0,0,.87));
font: 500 12px/1 var(--md-text-font, system-ui, -apple-system, Segoe UI, Roboto, Ubuntu, Cantarell, Noto Sans, sans-serif);
opacity: .72;
Copy link
Member

Choose a reason for hiding this comment

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

These seem to mostly repeat things that are already set in .kbd-hint. I'm not that well versed in CSS, is this really necessary? Why do we need them as two independent settings?

Copy link
Author

Choose a reason for hiding this comment

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

they’re separate because one is a DOM element and the other is a pseudo-element positioned inside the input. If we want both contexts covered (inside the field on desktop, next to the icon on mobile/when the field is hidden), we need both targets.

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by next to the icon on mobile / when the field is hidden? For me, when the field is hidden, the hint is hidden as well?

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat (enhancement) : on event cmd + k -> focus on search bar
2 participants