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

set attribute values for snapshotting input values #14

Closed

Conversation

cbroeren
Copy link

@cbroeren cbroeren commented Oct 3, 2016

Fix for snapshotting input values, issue #6.
Not sure this is the best way to do this

Copy link
Contributor

@fotinakis fotinakis left a comment

Choose a reason for hiding this comment

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

This is great! My thoughts here are that I would like to make sure this is tested for a few more element types and scoped down a little bit, for example to make sure that it works correctly for text inputs, checkboxes, radios, and maybe select boxes (may already work?), but does not affect <input type="button"> or submit, etc which I think this would. Also, would be nice to use the same mechanism to fix textarea.

So, I think the changes are:

  • Add a bunch more input types in the test render call.
  • Scope down dom.find('input') to something more like dom.find('input[type=text],input[type=checkbox],textarea')
  • I'm fine with the abstracted jQuery methods like .val()/.prop(), maybe .each for consistency with the rest of the code.

@cbroeren
Copy link
Author

cbroeren commented Oct 4, 2016

You're absolutely right. Missing a lot of cases. Will improve it before the end of the week.

@fotinakis
Copy link
Contributor

Hey @cbroeren, have any time to finish this out? No worries if not, just checking—thinking about dedicating some time to fix this.

@cbroeren
Copy link
Author

cbroeren commented Dec 5, 2016

@fotinakis, No I'm sorry, I started it, but I'm much busier then expected. Excuse me for not fulfilling my promise. For me it will take a while to make time for this. Would be nice when it will be fixed though, really appreciate it.

@kiwiupover
Copy link

@fotinakis FYI I added a PR to ember-svg-jar evoactivity/ember-svg-jar#33

When embedding svgs in the document they are put into the body of the document by default. My PR allows the user to move the injected object inside the #ember-testing-container.

Will this PR change the percy snapshot root element from #ember-test -> #ember-testing-container ?

As I type master is using #ember-test

Cheers
Dave

PS have you had post-it notes in your salad lately?

@fotinakis
Copy link
Contributor

@kiwiupover This PR will not affect that, no. :) I've commented on your SVG PR, that would definitely fix the issue you were having. We should create a separate ember-percy issue for adding a onSnapshot hook that can modify the DOM.

@bennettmt
Copy link
Contributor

@cbroeren @fotinakis I have some updates to this PR that I would like to commit that addresses some of the requested changes. What is the best way to do so?

@fotinakis
Copy link
Contributor

Hey @bennettmt — cool! This PR is pretty out of date at this point, though the idea is still solid. Can you just create another PR?

@fotinakis
Copy link
Contributor

Going to close this one as it's gotten stale, but leaving #6 open.

@fotinakis fotinakis closed this Jun 2, 2017
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

4 participants