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

Merge in class filtering solution from issue 336 #343

Merged
merged 7 commits into from
May 7, 2020

Conversation

emanguy
Copy link
Contributor

@emanguy emanguy commented Jan 27, 2020

Summary

Adds in filtering for contents of attributes as proposed in #336 (thanks for the sample code @ravishivt !)

Fixes #336

Test Plan

Tested by adding into storyshots tests for my company, but additional automated testing would be great. Please advise on what I should do for that. Sample from one of the tests:

image

Update: the example app snapshot test verifies this functionality.

What's required for testing (prerequisites)?

What are the steps to reproduce (after prerequisites)?

Compatibility

OS Implemented
iOS
Android

(assuming the other serializer code works, this should too)

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md
  • I mentioned this change in CHANGELOG.md
  • I updated the typed files (TS and Flow)
  • I added a sample use of the API in the example project (example/App.js)

@emanguy
Copy link
Contributor Author

emanguy commented Jan 27, 2020

The test above failed because the no NG attributes serializer removed the auto-added angular classes as expected. Don't know if that helps the case for the fact that this works.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 3, 2020

  • I think you can update snapshot of the failed test.
  • Is it possible to remove ng-star-inserted in this PR too ?

@emanguy
Copy link
Contributor Author

emanguy commented Feb 6, 2020

How do I update the snapshot? I didn't see anything referencing the snapshots in the npm scripts

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 6, 2020

Hi, you can do this:

  • cd examples
  • yarn test -u

Indeed there is no update snapshot script but you just need to use jest cli option.

@emanguy
Copy link
Contributor Author

emanguy commented Feb 11, 2020

Ah, I didn't realize there was a separate NPM project in the example directory. I've made the changes you requested.

src/AngularNoNgAttributesSnapshotSerializer.js Outdated Show resolved Hide resolved
src/AngularNoNgAttributesSnapshotSerializer.js Outdated Show resolved Hide resolved
example/src/app/__snapshots__/app.component.spec.ts.snap Outdated Show resolved Hide resolved
@wtho wtho added the Revision needed This PR needs code changes before it can be merged label Mar 12, 2020
@emanguy
Copy link
Contributor Author

emanguy commented Apr 14, 2020

Sorry this has gotten stale. I'll get around to making the changes this weekend.

@emanguy
Copy link
Contributor Author

emanguy commented Apr 18, 2020

Alright! Changes are made. The regexes are now more targeted at things following those prefixes.

@emanguy emanguy requested a review from wtho April 18, 2020 20:47
Copy link
Collaborator

@wtho wtho left a comment

Choose a reason for hiding this comment

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

Awesome! Looks great! Thanks so much!
Can you add an entry to CHANGELOG.md so we can merge it directly?

@ahnpnl please have another look and we can get this merged.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Apr 21, 2020

LGTM, only needs to resolve the conflict and add CHANGELOG.md 👍

Probably this also needs to be ivy compatible, but that will be for another PR imo.

@intellix
Copy link
Contributor

intellix commented May 6, 2020

How about also this which creeped in since updating to 8.1.3 with ivy support:

  __ngContext__={[Function LRootView]}

Also, you said ivy compatible can come after but that support is already in master so... required?

@ahnpnl
Copy link
Collaborator

ahnpnl commented May 6, 2020

The current snapshot serializer in 8.1.3 is already compatible with ivy. This PR is only an enhancement to get rid of unwanted attributes :)

@emanguy
Copy link
Contributor Author

emanguy commented May 7, 2020

Changelog is updated, I'm hoping you intended for me to put it in the 8.1.3 changes!

Merge conflict is also resolved.

@wtho wtho merged commit bcb2c22 into thymikee:master May 7, 2020
@wtho
Copy link
Collaborator

wtho commented May 7, 2020

No, 8.1.3 is already released. I will fix in on the 8.2 release though.
Thanks a lot!

@wtho wtho removed the Revision needed This PR needs code changes before it can be merged label May 7, 2020
@wtho
Copy link
Collaborator

wtho commented May 7, 2020

Released in v8.2.0.

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.

Remove more internal angular attributes from snapshots
4 participants