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(theme): add slash to hotKey search #2328

Merged

Conversation

viniciusteixeiradias
Copy link
Contributor

What is the purpose of this pull request?

  • Bug fix
  • New feature
  • Documentation update
  • Other

Solving this problem here (Vuejs).

@brc-dd
Copy link
Member

brc-dd commented May 1, 2023

/ needs specially handling. E.g. if you've a live code component in your page and you're typing slash there, it should not open the search box. We should copy the logic from here onKeyDown, isEditingContent

@viniciusteixeiradias
Copy link
Contributor Author

viniciusteixeiradias commented May 1, 2023

@brc-dd Okay, The validation you referred to was done for the following reason:

Code here

// The `/` shortcut opens but doesn't close the modal because it's a character.
         (!isEditingContent(event) && event.key === '/' && !isOpen)

But in our case the / shortcut is not closing the modal when we type it in the search bar.

@brc-dd
Copy link
Member

brc-dd commented May 2, 2023

That comment is for !isOpen check not !isEditingContent. Your PR will not behave as expected when there is any content editable or input/textarea element in the page and the user is typing slash there. (This PR opens the search box in those cases but it should not do that.)

@viniciusteixeiradias
Copy link
Contributor Author

@brc-dd I did not understand your comment, could you explain again?

This treatment was already necessary before the opening of my PR, since the opening with / worked after the first opening. My PR was objective, making it possible to open the dialog with / after rendering the component.

Suppose treatment should exist, but is it something that should be done in this PR?

Obs: I tested with input, text-area and select, slash only opened the dialog when typed in the text-area.

@brc-dd
Copy link
Member

brc-dd commented May 2, 2023

@viniciusteixeiradias

Screen.Recording.2023-05-02.at.10.30.27.AM.mov

@viniciusteixeiradias
Copy link
Contributor Author

@brc-dd Sorry for my mistake, I performed the test before the build so it worked for me. It's my first time working in open source, I'm learning with you. Anyway, I uploaded the corrections

@viniciusteixeiradias
Copy link
Contributor Author

@brc-dd All good now?

@brc-dd brc-dd self-assigned this May 8, 2023
@brc-dd brc-dd merged commit c20bd28 into vuejs:main May 8, 2023
8 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants