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(button): use kbd instead of span #1335

Merged
merged 2 commits into from
Apr 17, 2022
Merged

Conversation

serjan-nasredin
Copy link
Contributor

Replaced span tag to kbd.

@netlify
Copy link

netlify bot commented Mar 24, 2022

Deploy Preview for docsearch canceled.

Name Link
🔨 Latest commit 383baff
🔍 Latest deploy log https://app.netlify.com/sites/docsearch/deploys/625bf4f3766af90008e59102

@codesandbox-ci
Copy link

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:

Sandbox Source
Vanilla Configuration

Comment on lines 50 to 59
<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>

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>
      )}

Copy link
Member

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

Copy link
Member

@shortcuts shortcuts left a 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 :)

Comment on lines 50 to 59
<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>
Copy link
Member

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 shortcuts changed the title Update DocSearchButton.tsx fix(button): use kbd instead of span Apr 17, 2022
@shortcuts shortcuts enabled auto-merge (squash) April 17, 2022 11:14
@shortcuts shortcuts disabled auto-merge April 17, 2022 11:16
@shortcuts shortcuts merged commit 7459a42 into algolia:next Apr 17, 2022
@serjan-nasredin serjan-nasredin deleted the patch-1 branch April 17, 2022 15:23
@tnir
Copy link
Contributor

tnir commented Jul 21, 2022

@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

button: use kbd instead of span (#1335) (7459a42)

@shortcuts
Copy link
Member

@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 😉

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.

@tnir
Copy link
Contributor

tnir commented Jul 21, 2022

Okay, I got the package structure now. Thank you for clarification.

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.

4 participants