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(v-dialog): adds click:outside call #6719

Merged

Conversation

rkomiyama
Copy link
Contributor

@rkomiyama rkomiyama commented Mar 13, 2019

Description

Adds an emitter which emits click:outside when the closeConditional() gets called.

Duplicate of this PR: #5593

Motivation and Context

Fixes #5533

How Has This Been Tested?

A new unit test has been created to check if click:outside has been called.

Markup:

<template>
  <div id="app">
    <v-app>
      <v-content>
        <v-container grid-list-xl>
          <v-layout row justify-center>
            <v-dialog
              v-model="open"
              persistent
              @click:outside="callback"
              width="300"
            >
              <v-btn slot="activator"> Open </v-btn>
              <v-card>
                My Dialog Content
              </v-card>
            </v-dialog>
            <v-dialog
              v-model="areYouSureDialog"
              persistent
              width="300"
            >
              <v-card>
                <v-flex>
                  Are you sure?
                </v-flex>
                <v-flex>
                  <v-btn @click="closeBoth"> Yes </v-btn>
                  <v-btn @click="areYouSureDialog = false"> No </v-btn>
                </v-flex>
              </v-card>
            </v-dialog>
          </v-layout>
        </v-container>
      </v-content>
    </v-app>
  </div>
</template>

<script>
export default {
  name: 'app',
  components: {
  },
  methods: {
    callback() {
      this.areYouSureDialog = true;
    },
    closeBoth() {
      this.open = false;
      this.areYouSureDialog = false;
    }
  },
  data: () => ({
    open: false,
    areYouSureDialog: false
  })
}
</script>

<style lang="scss" scoped>
#app {
  font-family: 'Avenir', Helvetica, Arial, sans-serif;
  -webkit-font-smoothing: antialiased;
  -moz-osx-font-smoothing: grayscale;
  text-align: center;
  color: #2c3e50;
  margin-top: 60px;
}
</style>

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

@codecov
Copy link

codecov bot commented Mar 13, 2019

Codecov Report

Merging #6719 into next will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #6719      +/-   ##
==========================================
+ Coverage   81.85%   81.92%   +0.07%     
==========================================
  Files         329      329              
  Lines        8438     8439       +1     
  Branches     2138     2138              
==========================================
+ Hits         6907     6914       +7     
+ Misses       1440     1434       -6     
  Partials       91       91
Impacted Files Coverage Δ
packages/vuetify/src/components/VDialog/VDialog.ts 66.66% <100%> (+3.83%) ⬆️
packages/vuetify/src/mixins/stackable/index.ts 100% <0%> (+10.52%) ⬆️

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 59bbe2a...1d33aa4. Read the comment docs.

Copy link
Contributor

@dsseng dsseng left a comment

Choose a reason for hiding this comment

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

@rkomiyama looks like you have opened feature PR to master, but now all features should go to next. Could you please change base to next and resolve merge conflicts if they happen.
Thank you for your contribution and interest in improving Vuetify! Make sure to join us in the Discord community.

@johnleider johnleider changed the base branch from master to next March 18, 2019 00:50
@johnleider
Copy link
Member

I've updated the target branch to next. This looks okay and as v-dialog hasn't been touched yet I'm okay with this going in. @vuetifyjs/core-team any objections?

@nekosaur
Copy link
Member

Fine with me

@MajesticPotatoe
Copy link
Member

sounds good.

@johnleider
Copy link
Member

Fix merge conflicts and we'll approve.

@johnleider johnleider self-assigned this Mar 31, 2019
@johnleider johnleider added T: feature A new feature S: has merge conflicts The pending Pull Request has merge conflicts labels Mar 31, 2019
@johnleider johnleider added this to the v2.0.0 milestone Mar 31, 2019
@rkomiyama rkomiyama force-pushed the feat/v-dialog-emit-outside-click branch from 118df19 to 1e3226e Compare April 2, 2019 15:59
@vercel
Copy link

vercel bot commented Apr 2, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Copy link
Contributor

@dsseng dsseng left a comment

Choose a reason for hiding this comment

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

@rkomiyama looks like there still are some merge conflicts

@rkomiyama
Copy link
Contributor Author

rkomiyama commented Apr 2, 2019

@rkomiyama looks like there still are some merge conflicts

Yeah I'm not sure why. I rebased with the latest dev but it's not showing any merge conflicts on my end :\

Screen Shot 2019-04-02 at 12 27 15 PM

@rkomiyama
Copy link
Contributor Author

Could someone please click on the "Resolve conflicts" button to see what exactly is causing this? I feel like this might be an issue of GitHub and Git comparing branches in different ways.

@KaelWD
Copy link
Member

KaelWD commented Apr 3, 2019

Rebase onto next, not dev

@TravisBuddy
Copy link

TravisBuddy commented Apr 3, 2019

Hey @rkomiyama,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: dfc03d80-5624-11e9-8811-9d5afad75740

@rkomiyama
Copy link
Contributor Author

rkomiyama commented Apr 3, 2019

There seems to be some issues with the existing tests in the next branch.

Edit: Never mind, it was an issue with not using the new mountFunction.

Copy link
Member

@MajesticPotatoe MajesticPotatoe left a comment

Choose a reason for hiding this comment

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

There seems to be some issues with the existing tests in the next branch.

Along side the few other edits that need to be done in the review (these are all converts to vue-test-utils)
Seems like you need mocks for theme. (see comment below about const wrapper = mountFunction({

@MajesticPotatoe MajesticPotatoe removed the S: has merge conflicts The pending Pull Request has merge conflicts label Apr 3, 2019
@rkomiyama rkomiyama force-pushed the feat/v-dialog-emit-outside-click branch from 5b6e66a to f125a96 Compare April 3, 2019 15:19
@vercel vercel bot temporarily deployed to staging April 3, 2019 15:19 Inactive
Copy link
Member

@MajesticPotatoe MajesticPotatoe left a comment

Choose a reason for hiding this comment

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

👍

@MajesticPotatoe MajesticPotatoe merged commit aa64854 into vuetifyjs:next Apr 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T: feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants