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

added check for componentOptions existing on slot fixes #6901 #6905

Merged
merged 2 commits into from Apr 13, 2019

Conversation

ricardovanlaarhoven
Copy link
Contributor

@ricardovanlaarhoven ricardovanlaarhoven commented Apr 3, 2019

Description

Adding a typeof !== 'undefined' check so only slots with componentOptions will be adding an animation.

Motivation and Context

it fixes #6901

How Has This Been Tested?

I've tested this in my project with the same code as the issues codepen: https://codepen.io/anon/pen/BENVJK?editors=1111

and with the playground as documented below

Markup:

Playground.vue
<template>
  <v-app>
    <child>
      <div slot="speedDialAfter">
        test
      </div>
    </child>
  </v-app>
</template>

<script>
  import Child from './Child.vue';
  export default {
    components: {Child},
    data: () => ({
    })
  }
</script>


Child.vue

<template>
  <div>
    <v-fab-transition>
      <v-speed-dial
          v-model="fab"
          :top="true"
          :right="true"
          :open-on-hover="true"
          direction="bottom"
          transition="slide-y-reverse-transition"
      >
        <v-btn slot="activator" color="accent" dark fab hover v-model="fab">
          <v-icon>view_headline</v-icon>
          <v-icon>close</v-icon>
        </v-btn>
        <v-tooltip left>
          <v-btn
              fab
              dark
              small
              color="red"
              slot="activator"
          >
            <v-icon>delete</v-icon>
          </v-btn>
          <span>delete</span>
        </v-tooltip>
        <slot name="speedDialAfter"></slot>
      </v-speed-dial>
    </v-fab-transition>
  </div>
</template>

<script>
  export default {
    name: 'child',
    data: () => ({
      fab: null
    })
  }
</script>

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

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)

No documentation needed

@vercel
Copy link

vercel bot commented Apr 3, 2019

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

@codecov
Copy link

codecov bot commented Apr 3, 2019

Codecov Report

Merging #6905 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #6905   +/-   ##
======================================
  Coverage    85.3%   85.3%           
======================================
  Files         298     298           
  Lines        7221    7221           
  Branches     1804    1804           
======================================
  Hits         6160    6160           
  Misses        960     960           
  Partials      101     101
Impacted Files Coverage Δ
...es/vuetify/src/components/VSpeedDial/VSpeedDial.js 96.15% <100%> (ø) ⬆️

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 189cbd2...03255c7. Read the comment docs.

@@ -65,7 +65,7 @@ export default {
if (this.isActive) {
let btnCount = 0
children = (this.$slots.default || []).map((b, i) => {
if (b.tag && b.componentOptions.Ctor.options.name === 'v-btn') {
if (b.tag && typeof b.componentOptions !== 'undefined' && b.componentOptions.Ctor.options.name === 'v-btn') {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (b.tag && typeof b.componentOptions !== 'undefined' && b.componentOptions.Ctor.options.name === 'v-btn') {
if (b.tag && typeof b.componentOptions && b.componentOptions.Ctor.options.name === 'v-btn') {

Copy link
Member

Choose a reason for hiding this comment

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

Should probably be any non-empty vnode, dunno why it singles out buttons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnleider is this typescript specific?
in javascript typeof something results in a string 'undefined'
And then it would always results in true because it's a string.

Anyway the code you suggested results in the same error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KaelWD Could you tell me how to do that? Your solution sounds better, if i understand it correctly you mean that we change the if to check if there is any vuetify component (starts with v-)

Perhaps we should ask @kuromoka and @sh7dm if there was any particular reason for this

dunno why it singles out buttons.

Copy link
Member

Choose a reason for hiding this comment

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

My suggested change was merely for style purposes. As far as @KaelWD 's suggestion. This is used in a few places, maybe it's time to normalize the functionality into a helper of sorts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi John,

But when i apply your auggestion and test it, it breaks.

Copy link
Member

Choose a reason for hiding this comment

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

check if there is any vuetify component (starts with v-)

any element, not just vuetify components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How could i get this PR accepted?
there is a lot of discussion and good suggestions, but nothing specific.

John's comment breaks code and i don't get what is better about it since only !== 'undefined' is removed.

Kael's comment is a really good suggestion, but i don't know how to implement that.

Choose a reason for hiding this comment

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

I'm facing same issue and code change above fixes it

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion was just to change the compare logic. If it doesn't work, so be it.

As far as getting this accepted, I'd say that this works and let's get it in.

@johnleider johnleider added the pending review The issue is still pending disposition label Apr 6, 2019
@johnleider johnleider added T: bug Functionality that does not work as intended/expected and removed pending review The issue is still pending disposition labels Apr 13, 2019
@dsseng dsseng merged commit 58a8725 into vuetifyjs:master Apr 13, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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] v-speed-dial error when using slots
5 participants