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

Use csstype for standardized CSS typings #411

Merged
merged 6 commits into from
Apr 13, 2018
Merged

Use csstype for standardized CSS typings #411

merged 6 commits into from
Apr 13, 2018

Conversation

pelotom
Copy link
Contributor

@pelotom pelotom commented Apr 13, 2018

Resolves #408. See that issue for details and rationale.

tikotzky
tikotzky reviewed Apr 13, 2018
View reviewed changes
typings/svg-properties.d.ts
zoomAndPan?: string
}
export interface SVGPropertiesCompleteSingle
extends CSS.SvgProperties<number | string> {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be CSS.SvgPropertiesFallback?
It seems like it would need to be in order to allow using arrays to set fallback values.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It does look like it was not supporting array until now, though i'm pretty sure that glamor supports it...

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 would have used CSS.SvgPropertiesFallback if I were writing these typings from scratch, but to minimize breakage I was trying to keep as many of the existing exports intact as possible. So this is accomplishing what CSS.SvgPropertiesFallback would:

type SVGPropertiesComplete = SingleOrArray<
  SVGPropertiesCompleteSingle,
  keyof SVGPropertiesCompleteSingle
>

@pelotom
Copy link
Contributor Author

pelotom commented Apr 13, 2018

I'm not sure what all the test failures are about, but these:

[test:cover]     - test/should-fail.test.tsx(289,35): error TS2322: Type '{ display: "blocks"; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<Component<object & CSSProperties & ExtraGlamorousP...'.
[test:cover]     -   Type '{ display: "blocks"; }' is not assignable to type 'Readonly<object & CSSProperties & ExtraGlamorousProps>'.
[test:cover]     -     Types of property 'display' are incompatible.
[test:cover]     -       Type '"blocks"' is not assignable to type '"none" | "ruby" | "table" | "inherit" | "initial" | "unset" | "contents" | "block" | "inline" | "...'.

[test:cover]     - test/should-fail.test.tsx(293,29): error TS2322: Type '{ display: "blocks"; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<Component<HTMLProps<HTMLDivElement> & object & CSS...'.
[test:cover]     -   Type '{ display: "blocks"; }' is not assignable to type 'Readonly<HTMLProps<HTMLDivElement> & object & CSSProperties & ExtraGlamorousProps>'.
[test:cover]     -     Types of property 'display' are incompatible.
[test:cover]     -       Type '"blocks"' is not assignable to type '"none" | "ruby" | "table" | "inherit" | "initial" | "unset" | "contents" | "block" | "inline" | "...'.

should be resolved by frenic/csstype#8. In the meantime, it allows a string because otherwise you forbid valid multi-part values.

@Ailrun
Copy link
Contributor

Ailrun commented Apr 13, 2018

I think you should update negative tests and snapshots.

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #411   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     10           
  Lines         177    177           
  Branches       50     50           
=====================================
  Hits          177    177

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 da564f1...4958729. Read the comment docs.

Ailrun
Ailrun reviewed Apr 13, 2018
View reviewed changes
src/__tests__/__snapshots__/typescript.js.snap
@@ -38,13 +38,7 @@ test/should-fail.test.tsx(100,24): error TS2551: Property 'colors' does not exis
test/should-fail.test.tsx(111,3): error TS2344: Type 'PropsWithoutTheme' does not satisfy the constraint '{ theme: any; }'.
Property 'theme' is missing in type 'PropsWithoutTheme'.
test/should-fail.test.tsx(119,3): error TS2345: Argument of type 'StatelessComponent<object>' is not assignable to parameter of type '\\"tspan\\"'.
test/should-fail.test.tsx(134,3): error TS2345: Argument of type '(props: { theme: any; } & ExampleComponentProps & object) => { display: \\"none\\" | \\"hidden\\"; }' is not assignable to parameter of type 'StyleArgument<CSSProperties, { theme: any; } & ExampleComponentProps & object>'.
Copy link
Contributor

Choose a reason for hiding this comment

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

Those are in should-fail tests, so I think you should remove relevant tests too. I mean, line 134 in test/should-fail.test.tsx.

Copy link
Contributor Author

@pelotom pelotom Apr 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

These tests will be valid again once frenic/csstype#8 is fixed. Would it make sense to just add comments to the effect that the lines are only temporarily not being flagged with errors, pending that PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@pelotom Oh, sorry. I though frenic/csstype#8 (comment) says that 'string is valid', but it does not... Sorry for giving confusion.

@Ailrun
Copy link
Contributor

Ailrun commented Apr 13, 2018

Looks OK for me.

@kentcdodds kentcdodds merged commit 775dc66 into paypal:master Apr 13, 2018
@kentcdodds
Copy link
Contributor

Thanks folks! This is great to remove so much code!

@pelotom pelotom deleted the csstype branch April 13, 2018 21:58
@dependencies dependencies bot mentioned this pull request Apr 16, 2018
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tikotzky tikotzky tikotzky left review comments

@Ailrun Ailrun Ailrun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pelotom @Ailrun @codecov-io @kentcdodds @tikotzky