"data-is" attribute is being removed if you mount on the same element since v3.4.1 #2317

Closed
fvezzoso opened this Issue Apr 12, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@fvezzoso

fvezzoso commented Apr 12, 2017

  1. Describe your issue:

Since v3.4.1 if you mount a new tag on the same element that other tag is mounted since the mount for the new tag runs before the unmount for the old one, the data-is attribute with the new value is removed.

I think this is due to this line remAttr(el, IS_DIRECTIVE) that it was changed to fix #2311 on e4b93df

  1. Can you reproduce the issue?

http://plnkr.co/edit/AMd3PeRsX4rmIhA38Ugi

When you click on the tag1 button it mounts just fine (you see the red background), then you click the tag2 button and it mounts but the background (it should be blue) is missing since the data-is attribute that is used on the compiled styles is missing.

If you comment out line 12 (v3.4.1) and uncomment line 14 (v3.4.0) you will see that it works.

I think that we need to add back the check for mustKeepRoot before removing the attribute, that shouldn't affect the fix for #2311 but I might be wrong, let me know what you think and I can help with the pull request.

  1. On which browser/OS does the issue appear?

Chrome/MacOS but it's not browser/os related.

  1. Which version of Riot does it affect?

3.4.1

  1. How would you tag this issue?
  • Question
  • Bug
  • Discussion
  • Feature request
  • Tip
  • Enhancement
  • Performance

@GianlucaGuarini GianlucaGuarini added the bug label Apr 12, 2017

@fvezzoso

This comment has been minimized.

Show comment
Hide comment
@fvezzoso

fvezzoso Apr 12, 2017

@GianlucaGuarini let me know your thoughts and I can help out with a pull request, thanks!

fvezzoso commented Apr 12, 2017

@GianlucaGuarini let me know your thoughts and I can help out with a pull request, thanks!

@fvezzoso fvezzoso changed the title from `data-is` attribute is being removed if you mount on the same element since v3.4.1 to "data-is" attribute is being removed if you mount on the same element since v3.4.1 Apr 12, 2017

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Apr 12, 2017

Member

@fvezzoso I guess removing that line should be enough, I don't think it's really needed

Member

GianlucaGuarini commented Apr 12, 2017

@fvezzoso I guess removing that line should be enough, I don't think it's really needed

@fvezzoso

This comment has been minimized.

Show comment
Hide comment
@fvezzoso

fvezzoso Apr 12, 2017

Awesome, I'll create the PR first thing tomorrow morning, thanks!

Awesome, I'll create the PR first thing tomorrow morning, thanks!

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Apr 12, 2017

Member

remember the tests ;)

Member

GianlucaGuarini commented Apr 12, 2017

remember the tests ;)

@fvezzoso

This comment has been minimized.

Show comment
Hide comment
@fvezzoso

fvezzoso Apr 14, 2017

Thanks, sorry I didn't get a chance to work on the PR!

Thanks, sorry I didn't get a chance to work on the PR!

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