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

feat(VMenu): disable-keys prop #5138

Merged
merged 5 commits into from
Dec 18, 2018
Merged

feat(VMenu): disable-keys prop #5138

merged 5 commits into from
Dec 18, 2018

Conversation

jacekkarczmarczyk
Copy link
Member

@jacekkarczmarczyk jacekkarczmarczyk commented Sep 21, 2018

Description

Adds disable-keys prop to disable the keyboard event handlers used only when the v-menu is used as a real menu with selecting items by pressing up/down/enter etc

Motivation and Context

fixes #5103

How Has This Been Tested?

visually, unit

Markup:

<template>
  <v-app>
    <v-content>
      <v-container>
        <v-menu attach :close-on-content-click="false" :close-on-click="false" disable-keys>
          <v-btn slot="activator">Open</v-btn>
          <v-card class="pa-3" width="800">
            <v-text-field label="Focus me and press TAB"></v-text-field>
            <v-text-field></v-text-field>
          </v-card>
        </v-menu>
      </v-container>
    </v-content>
  </v-app>
</template>

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • 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 feature but make things better)

Checklist:

  • The PR title is no longer than 64 characters.
  • The PR is submitted to the correct branch (master for bug fixes, dev for new features and breaking changes).
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have created a PR in the documentation with the necessary changes.

@jacekkarczmarczyk jacekkarczmarczyk added T: bug Functionality that does not work as intended/expected T: feature A new feature labels Sep 21, 2018
@KaelWD KaelWD added S: has merge conflicts The pending Pull Request has merge conflicts and removed S: has merge conflicts The pending Pull Request has merge conflicts labels Oct 29, 2018
@jacekkarczmarczyk jacekkarczmarczyk removed the S: has merge conflicts The pending Pull Request has merge conflicts label Oct 29, 2018
@@ -13,6 +13,13 @@ import { keyCodes } from '../../../util/helpers'

/* @vue/component */
export default {
props: {
keyable: {
Copy link
Member

Choose a reason for hiding this comment

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

I know that we have older exceptions else where, but I tend to lean towards props that require boolean true to be active as opposed to writing :prop="false". Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

i'm ok with disableKeys prop (default: false)

@jacekkarczmarczyk jacekkarczmarczyk changed the title feat: v-menu keyable prop feat(VMenu): disable-keys prop Nov 1, 2018
@jacekkarczmarczyk jacekkarczmarczyk added the S: needs docs update The change requires an update to the documentation label Nov 1, 2018
@johnleider
Copy link
Member

This is tagged as a bug but submitted to the dev branch, could you provide additional insight?

@jacekkarczmarczyk
Copy link
Member Author

It introduces a new disable-keys prop

@KaelWD
Copy link
Member

KaelWD commented Nov 23, 2018

Alternative without a new prop: #5716

@johnleider johnleider added T: feature A new feature and removed T: feature A new feature labels Dec 2, 2018
@jacekkarczmarczyk jacekkarczmarczyk added this to the v1.4.0 milestone Dec 12, 2018
@codecov
Copy link

codecov bot commented Dec 14, 2018

Codecov Report

Merging #5138 into dev will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #5138      +/-   ##
==========================================
+ Coverage   88.52%   88.54%   +0.01%     
==========================================
  Files         281      281              
  Lines        6303     6303              
  Branches     1581     1581              
==========================================
+ Hits         5580     5581       +1     
+ Misses        605      604       -1     
  Partials      118      118
Impacted Files Coverage Δ
...uetify/src/components/VMenu/mixins/menu-keyable.js 95.45% <ø> (+4.54%) ⬆️
packages/vuetify/src/components/VMenu/VMenu.js 83.63% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4906f03...d8a587d. Read the comment docs.

@johnleider johnleider merged commit d6b3297 into dev Dec 18, 2018
@johnleider johnleider deleted the feat/#5103-v-menu-keyable branch December 18, 2018 15:11
@lock
Copy link

lock bot commented Apr 14, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. Please direct any non-bug questions to our Discord

@lock lock bot locked as resolved and limited conversation to collaborators Apr 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
S: needs docs update The change requires an update to the documentation T: bug Functionality that does not work as intended/expected T: feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants