-
Notifications
You must be signed in to change notification settings - Fork 398
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(button): use kbd instead of span #1335
Conversation
✅ Deploy Preview for docsearch canceled.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 4fd0905:
|
<span className="DocSearch-Button-Keys"> | ||
{key !== null && ( | ||
<> | ||
<span className="DocSearch-Button-Key"> | ||
<kbd className="DocSearch-Button-Key"> | ||
{key === ACTION_KEY_DEFAULT ? <ControlKeyIcon /> : key} | ||
</span> | ||
<span className="DocSearch-Button-Key">K</span> | ||
</kbd> | ||
<kbd className="DocSearch-Button-Key">K</kbd> | ||
</> | ||
)} | ||
</span> |
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.
+1 for using semantic HTML. I'd also assume the check for key
should happen one level higher as there's no point in rendering an empty span I'd imagine.
{key !== null && (
<span className="DocSearch-Button-Keys">
<kbd className="DocSearch-Button-Key">
{key === ACTION_KEY_DEFAULT ? 'Search' : key}
</kbd>
<kbd className="DocSearch-Button-Key">K</kbd>
</span>
)}
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 was the case before, but we now use the first span to prevent UI flashes: #1033
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.
Thanks, good idea! I've also added it to the footer keys :)
<span className="DocSearch-Button-Keys"> | ||
{key !== null && ( | ||
<> | ||
<span className="DocSearch-Button-Key"> | ||
<kbd className="DocSearch-Button-Key"> | ||
{key === ACTION_KEY_DEFAULT ? <ControlKeyIcon /> : key} | ||
</span> | ||
<span className="DocSearch-Button-Key">K</span> | ||
</kbd> | ||
<kbd className="DocSearch-Button-Key">K</kbd> | ||
</> | ||
)} | ||
</span> |
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 was the case before, but we now use the first span to prevent UI flashes: #1033
@shortcuts Thank you for the update, but from the following changelog, I could not find which package is updated. That could be just why I am not sure how monorepo handles everything (and this kind of problems) at all 😉 https://github.com/algolia/docsearch/blob/HEAD/CHANGELOG.md#310-2022-05-18
|
I believe the confusion on your side is because how the DocSearch library is built. We only implement things in the React package, because the JS one is an alias from React to Preact and does not have any logic in it. So it's either a change on React, which is automatically reflected on JS, or a CSS change. |
Okay, I got the package structure now. Thank you for clarification. |
Replaced span tag to kbd.