-
Notifications
You must be signed in to change notification settings - Fork 25
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
Issue 52 take #2 #76
Issue 52 take #2 #76
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @brunoocasali one small change needs. Could you please add info to changelog for newly added skip parameter and of course please add your name or nickname there. Other than that amazing job here. Thank you for valuable contribution. Appreciated. 🙏
@sinankeskin awesome! I did that :D (if you want some change please tell me) |
Hey @brunoocasali this test fails roughly 4/5 on locally both headless and regular browser. Tested on both Chrome and Chromium based Edge. Could you please take a look? Thanks. |
Could you give your ember/node versions? |
Yeah, unfortunately, I didn't make it to reproduce the behavior you showed to me :/
|
Dropped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good now. Ready to merge 🎉
I've created this PR #53 a year ago, after this time a lot of changes were introduced to this library, making that code useless for this new version.
To prevent anyone having their code-breaking because of a push force or something, I created a new branch/PR with the same idea but this time, working with the glimmer version.
In this new PR, I've made everything work as usual and added two use cases without the
@skip
option to make sure the old behavior is set.Closes #52
CC: @mateusalexandre and @sinankeskin