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(VCombobox): do not add search to list when selecting items with keyboard #9866

Merged
merged 11 commits into from
Jan 20, 2020
Merged

fix(VCombobox): do not add search to list when selecting items with keyboard #9866

merged 11 commits into from
Jan 20, 2020

Conversation

haggys22
Copy link
Contributor

@haggys22 haggys22 commented Dec 3, 2019

Description

There was a timing issue with this.$nextTick, triggering the selection twice: once for the selected VMenu item and once for the search term when the menu index is -1.

See #8841 (comment) for the code execution order without this fix.

Motivation and Context

This fixes #8841. Also the workaround for #8842 works again.

How Has This Been Tested?

visually

Markup

<template>
  <v-container>
    <v-combobox
            v-model="model"
            :items="items"
            :search-input.sync="search"
            hide-selected
            label="Add some tags"
            multiple
            persistent-hint
            chips
    ></v-combobox>
  </v-container>
</template>

<script>
export default {
  data: () => ({
    items: ['Gaming', 'Programming', 'Vue', 'Vuetify'],
    model: [],
    search: null,
  }),
  watch: {
    model() {
      this.search = null
    }
  }
}
</script>

Types of changes

  • Bugfix (non-breaking change which fixes a bug)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement/refactoring (non-breaking change that doesn't add any features but makes things better)

Checklist:

  • The PR title is no longer than 64 characters.
  • The PR is submitted to the correct branch (master for bug fixes and documentation updates, dev for new features and breaking changes).
  • My code follows the code style of this project.
  • I've added relevant changes to the documentation (applies to new features and breaking changes in core library)
  • I've added new examples to the kitchen (applies to new features and breaking changes in core library)

@TravisBuddy
Copy link

TravisBuddy commented Dec 3, 2019

Hey @haggys22,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: c7a9b3c0-3607-11ea-b480-9fec542541c3

@haggys22
Copy link
Contributor Author

haggys22 commented Dec 4, 2019

For some reason the test

FAIL Vuetify src/components/VSelect/__tests__/VSelect4.spec.ts
  ● VSelect.ts › should emit listIndex event when navigated by keyboard

    expect(jest.fn()).toHaveBeenCalledWith(expected)

    Expected mock function to have been called with:
      0
    as argument 1, but it was called with
      -1.

fails, but inspecting a VSelect with Vue devtools shows that update:list-index is emitted with the correct index when navigating with the keyboard

@johnleider johnleider added the S: has merge conflicts The pending Pull Request has merge conflicts label Dec 4, 2019
After triggering `click` the `VMenu` needs one tick to activate, therefore the `keydown.down` event during the same tick will be passed to the still inactive menu und not update the `listIndex`.

Waiting for the next tick fixes this as the menu will be active afterwards
@haggys22
Copy link
Contributor Author

haggys22 commented Dec 5, 2019

@johnleider

  • Updated branch
  • Added Playground.vue to PR message
  • Fixed failing test
  • Updated PR title/message as this does not even fix the issue but only the workaround

@haggys22 haggys22 changed the title Fix #8841 Fix workaround for #8841 Dec 5, 2019
@jacekkarczmarczyk jacekkarczmarczyk removed the S: has merge conflicts The pending Pull Request has merge conflicts label Dec 5, 2019
@johnleider
Copy link
Member

Testing your branch and the issue still persists.

select

@johnleider johnleider added C: VSelect VSelect T: bug Functionality that does not work as intended/expected labels Dec 17, 2019
@johnleider johnleider changed the title Fix workaround for #8841 fix(VSelect): do not add search to list when selecting items with keyboard Dec 17, 2019
@haggys22
Copy link
Contributor Author

With my latest changes the v1.5.18 behaviour is restored, i.e. after pressing enter only the selected item is added as a chip and the search term remains in the input field

Copy link
Member

@johnleider johnleider left a comment

Choose a reason for hiding this comment

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

Please add a corresponding unit test for this, thank you!

@johnleider johnleider changed the title fix(VSelect): do not add search to list when selecting items with keyboard fix(VCombobox): do not add search to list when selecting items with keyboard Jan 16, 2020
@johnleider
Copy link
Member

Determined that #8841 was a duplicate of #6697

@lock lock bot locked as resolved and limited conversation to collaborators Feb 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C: VSelect VSelect T: bug Functionality that does not work as intended/expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug Report] Combobox add 2 items when pressing enter on a search item
4 participants