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

chore: upgrade things #244

Closed
wants to merge 1 commit into from
Closed

chore: upgrade things #244

wants to merge 1 commit into from

Conversation

kentcdodds
Copy link
Contributor

@kentcdodds kentcdodds commented Jul 24, 2017

What: This updates a bunch of dependencies and things

Why: Just good to keep up to date (and no, I don't want to use greenkeeper)

How:

  • I removed yarn.lock because I've decided that lockfiles aren't good for libraries
  • Ran npx npm-check -u
  • Updated some stuff, will comment in-line

Checklist:

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

@@ -1,49 +1,49 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`overriding styles via className 1`] = `
.css-13vote6,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yay for v2 of jest-glamor-react!

@@ -1,7 +1,9 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Typescript expected failures 1`] = `
"test/should-fail.test.tsx(10,3): error TS2345: Argument of type '{ fillRule: \\"cat\\"; }' is not assignable to parameter of type 'StyleArgument<SVGProperties, {}, object>'.
"test/glamorous.test.tsx(175,31): error TS2322: Type 'HTMLDivElement | null' is not assignable to type 'HTMLDivElement'.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@luke-john, is this correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not no. I'm unsure what caused that, none of the dependencies look like they should have impacted this.

It doesn't appear to have occurred on #246 though, if it happens again I'll investigate further.

Also noticed that this test completely fails on windows due to inconstancies with spawnSync :(. I'll hopefully land something in the coming days to resolve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#246 was on master so it didn't have your recent changes.

@@ -40,10 +39,9 @@ test('can use pre-built glamorous components with css prop', () => {
const computed = 'background'
expect(
render(
<glamorous.A
<glamorous.Span
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated this to a Span because at some point, it was realized that A is a SVG element I guess (tada) which means that all valid SVG props can be forwarded to the A tag. This is actually quite unfortunate because we cannot actually tell ahead of time whether an A tag is used within an SVG or not. So when you use the display prop (as we did in this test), we cannot tell whether you want to forward it on or use it as CSS.

Today we forward it on rather than using it as CSS. It looks like this changed 11 days ago in react-html-attributes, and it's causing the display prop to forward instead of be applied as a css value.

Let's deal with this in a separate issue, but I think we may want to reconsider some things around how we handle this...

@kentcdodds
Copy link
Contributor Author

Going to do this with #246 because that'll be easier 🙄

@kentcdodds kentcdodds closed this Jul 24, 2017
@kentcdodds kentcdodds deleted the pr/upgrade-stuff branch July 24, 2017 23:46
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.

2 participants