Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

Include filter props #340

Merged
merged 6 commits into from
Oct 20, 2017
Merged

Include filter props #340

merged 6 commits into from
Oct 20, 2017

Conversation

johnjesse
Copy link
Contributor

What:

Added filterProps to the glamorousOptions interface and the propsAreCSSOveridesOptions interface.
Also changed the forwardProps on both interfaces so it is typed to string[]

Why:

They are methods missing on the interfaces, and are causing compiler errors when running with typescript

How:

Checklist:

  • Documentation N/A
  • Tests
  • Ready to be merged
  • Added myself to contributors table

I added the pre-commit line in the opt-in file, but didn't actually get told why my commits failed so I ended up commiting under the -no-verify flag.

@johnjesse johnjesse changed the title Include filter props (#339) Include filter props Sep 28, 2017
@johnjesse
Copy link
Contributor Author

Failed because the snapshot was updated? But the contributing log said to run 'npm run test:update' and commit any snapshot changes...

Also, I don't really understand why the snapshot changed when I only added types to some interfaces

@kentcdodds kentcdodds closed this Sep 28, 2017
@kentcdodds
Copy link
Contributor

Whoops! Wrong button!

@kentcdodds kentcdodds reopened this Sep 28, 2017
@kentcdodds
Copy link
Contributor

The snapshot changes look totally unrelated to your changes. I'm not sure why you needed to update the snapshots locally 🤔

@johnjesse
Copy link
Contributor Author

I'll back them out, but here https://github.com/paypal/glamorous/blob/master/CONTRIBUTING.md it does say to run the tests locally, I guess I misunderstood the direction and you only need to run with the update flag if you are meant to be updating snapshots?

@kentcdodds
Copy link
Contributor

No, I think you didn't do anything wrong. There's something messed up with glamorous's tests right now. I'm working on it.

@kentcdodds
Copy link
Contributor

I'm afraid that I've run out of time to work on this. I'll try to get back to it eventually, but I can't make any promises (we just had baby # 4 and my kids are wondering why daddy isn't playing with them while Mom takes care of the baby).

If anyone wants to step in here to figure out what's going on that'd be great.

@luke-john
Copy link
Collaborator

Just had a look into this, the snapshot mismatchs look like they are related to the version of node you run the tests with.

If you run them with node 6 you get the output as seen in this PR, if you run them with node 8 you get the output as seen on the current master branch.

@johnjesse would you be able to rerun the tests with node 8 to redo the snapshots?

@johnjesse
Copy link
Contributor Author

@luke-john, thanks for looking into it. I'll re-run the tests with node 8 as soon as I get the chance

@johnjesse
Copy link
Contributor Author

Hey, so nothing has happened on this for a while, I know the checks haven't reported, do you want me to do anything else, squash the commits etc...

Copy link
Contributor

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@kentcdodds kentcdodds merged commit 0cc1006 into paypal:master Oct 20, 2017
@codecov-io
Copy link

Codecov Report

Merging #340 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #340   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     10           
  Lines         174    174           
  Branches       48     48           
=====================================
  Hits          174    174

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a05a78...193b17c. Read the comment docs.

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

Successfully merging this pull request may close these issues.

None yet

5 participants