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/support html attributes #61

Merged
merged 10 commits into from
Aug 24, 2018

Conversation

nlitwin
Copy link
Contributor

@nlitwin nlitwin commented Aug 5, 2018

Description

This pull requests adds support for passing down regular HTML attributes like tabindex and disabled from the ngVue component to the Vue root element. Please see original issue described in #60 for an example.

Todos

  • regular attributes (<button tabindex="1"></button>)
  • attributes without values (<button disabled></button>)
  • updating value based on scope changes (<button tabindex="ctrl.myTabindex"></button>)
  • support data- attributes (Angular normalizes them and strips the data- part out by default)
  • ensure that inheritAttrs: false and binding $attrs to a non-root element works
  • add unit tests
  • update docs

Notes

In working on this PR, I noticed a few places that check for either vProps or vData, but as far as I could tell, vData is just an alias. Is it needed? If not, I think the code could be cleaned up a bit and some of the utility functions like getExpressions/watchExpressions and if/else blocks could be simplified. Let me know! If vData isn't needed, I'm happy to remove it and refactor.

@nlitwin
Copy link
Contributor Author

nlitwin commented Aug 12, 2018

@nicolaspayot heya, let me know if there're any improvements I can make - thanks!

@nicolaspayot
Copy link
Member

@nlitwin thanks for you work, it looks really interesting. Sorry for the late answer, I was on holiday. Currently reviewing your PR!

@nicolaspayot
Copy link
Member

@nlitwin LGTM!

However, I think there's now more complexity in some parts of the code because of the introduction of several if/else blocks, such as in files src/components/props/getExpressions.js and src/components/props/watchExpressions.js. I think it kinda breaks the original flow. I don't know if I'm clear 😅but anyway, not a big deal. And I don't know yet how to write it in another way...

Concerning vData, it is indeed an alias for vProps and I never used it, but maybe there are developers that are using it. It would be brutal to remove it in this PR. But we could totally deprecate it in another one and then remove it and bump major version.

@nlitwin
Copy link
Contributor Author

nlitwin commented Aug 24, 2018

@nicolaspayot Thank you for your time and taking a look!

I agree that the if/else blocks add some complexity. I was trying to avoid it, but I didn't see another way of doing it without really changing the structure of the watch/watchExpressions functions. I think the main issue was that I needed both props and attrs on reactiveData, and the watch function in watchExpressions.js needed to know which property to work on (reactiveData._v[type]).

I saw there was a small merge conflict, so I fixed that. If there's anything else, just let me know! 😄

@nicolaspayot
Copy link
Member

nicolaspayot commented Aug 24, 2018

@nlitwin yes, I've merged PR #64 into master, which created a small conflict. Awesome, thank you!

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

Successfully merging this pull request may close these issues.

None yet

2 participants