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

vue/require-toggle-inside-transition - question: should it still be an error with a conditional appear attribute? #2467

Closed
saifahn opened this issue May 28, 2024 · 8 comments · Fixed by #2487

Comments

@saifahn
Copy link

saifahn commented May 28, 2024

What rule do you want to change?

  • vue/require-toggle-inside-transition

Does this change cause the rule to produce more or fewer warnings?

  • fewer

How will the change be implemented? (New option, new default behavior, etc.)?

  • new default behaviour - don't give an error when there is a conditional appear attribute on a node

Please provide some example code that this change will affect:

<transition :appear="appIsLoaded">
  <div>Something</div>
</transition>

What does the rule currently do for this code?

  • shows an error on the div - "The element inside <transition> is expected to have a v-if or v-show directive"

What will the rule do after it's changed?

  • not show an error

Additional context

We are using a conditional appear attribute so that the animation is not shown on the first load of the app, only when navigating between components.

I'm not 100% sure this is a valid use of appear and transition, so I would like to confirm this too.

Thanks!

@saifahn
Copy link
Author

saifahn commented Jun 4, 2024

If a conditional appear attribute should be sufficient for this rule, I can take a look at making a PR for it 🙂

@FloEdelmann
Copy link
Member

We are using a conditional appear attribute so that the animation is not shown on the first load of the app, only when navigating between components.

Why not set :appear="false" (or leave it out) and then use v-if/v-else on the element that should be transitioned (i.e. <div> in you example)?

@saifahn
Copy link
Author

saifahn commented Jun 4, 2024

Unfortunately, it wouldn't work in our case because it's essentially like this:

<transition :appear="isNotInitialLoad">
  <div>
    <some-component />
  </div>
</transition>

isNotInitialLoad comes from the store, and is set after the first load is complete.

This is not a top level component - we have others at the same level so there is navigation to and from others that are like this with a conditional appear based on isInitialLoad.

There is an animation when going between pages but we don't want this to appear on the first load because it looks a little strange.

Maybe we're approaching how to do this a little wrong, and there should be some transitions at a higher level but it might be a quite big refactor with how it's set up right now.

@FloEdelmann
Copy link
Member

Hmm, I'm not sure whether this kind of usage should be encouraged by not reporting an error. @ota-meshi what do you think?

@ota-meshi
Copy link
Member

Hmm. In my opinion, <transition> with an :appear directive should exclude reporting. Because <transition :appear="true"> will also be a false positive.

@FloEdelmann
Copy link
Member

Okay, then this is open for contributions. If anyone is interested, a PR is welcome!

Please add these test cases:

<!-- PASS -->
<transition appear><div /></transition>
<transition :appear="foo"><div /></transition>
<transition :appear="true"><div /></transition>

<!-- FAIL -->
<transition :appear="false"><div /></transition>

@ota-meshi
Copy link
Member

<transition appear><div /></transition>

The appear attribute (not :appear) is already in the test case, so there’s no need to add it 😉

code: '<template><transition appear><div /></transition></template>'

@saifahn
Copy link
Author

saifahn commented Jun 10, 2024

If it's OK, I'll hopefully take a look at this some time in the not too distant future - maybe this week or next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants