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

Add CSSFunctions to CSSProperties typings #392

Merged
merged 5 commits into from
Mar 12, 2018

Conversation

quicksnap
Copy link
Collaborator

What:

Improves TypeScript support for CSS functions

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

@codecov-io
Copy link

codecov-io commented Mar 3, 2018

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #392   +/-   ##
=====================================
  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 8a21d8b...7fb508a. Read the comment docs.

kentcdodds
kentcdodds previously approved these changes Mar 3, 2018
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.

LGTM
but I don't merge typescript changes 😅

luke-john
luke-john previously approved these changes Mar 3, 2018
Copy link
Collaborator

@luke-john luke-john left a comment

Choose a reason for hiding this comment

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

Thanks for adding support for this (and adding the feature in the first place 👍), should enable some handy patterns.

Would you be able to add a test to /test/glamorous.test.tsx which makes use of this type addition.

Also, from the looks of the PR that landed this feature, the shape is also supported for the glamorous style arguments so we probably need to update StyleFunction in style-arguments.d.ts to include itself in it's return union.

ie.

export interface StyleFunction<Properties, Props> {
  (props: Props):
    | Properties
    | string
    | Array<Properties | string | StyleFunction<Properties, Props>>
    | StyleFunction<Properties, Props>
}

feel free to leave out of scope and I'll setup up a first timer issue for that.

Also re my comment around the CSSFunction location, also feel free to leave as is, if I get some time in the next week I'll look into adding the generic support to type the Props, or someone else will no doubt get onto it :).

Thanks again.

| CSSFunction

// TODO: This could be made generic. Issue PR if you're so inclined!
export interface CSSFunction {
Copy link
Collaborator

Choose a reason for hiding this comment

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

While this will work well, it might be easier to add support in the glamorous-component.ts file which would also make it simpler to add the typing for the Props argument and help keep the logic of that function in one file.

// glamorous-component.d.ts

export interface CSSFunction<Props> {
    (props: Props): CSSPropertiesRecursive | CSSFunction<Props> 
}

export interface ExtraGlamorousProps<Props> {
  ...
  css?: CSSPropertiesRecursive | CSSFunction<Props>
  ...
}
...
export interface GlamorousComponent<ExternalProps, Props>
  extends React.ComponentClass<ExternalProps & ExtraGlamorousProps<ExternalProps>>,
    GlamorousComponentFunctions<ExternalProps, Props> {}
...
export type GlamorousComponentProps<ExternalProps> = JSX.IntrinsicAttributes &
  JSX.IntrinsicClassAttributes<
    React.Component<ExtraGlamorousProps<ExternalProps> & ExternalProps, React.ComponentState>

@quicksnap quicksnap dismissed stale reviews from luke-john and kentcdodds via 8b6b372 March 5, 2018 17:26
@quicksnap
Copy link
Collaborator Author

@luke-john I've added a TypeScript test for this PR.

Let's open separate tickets for the remaining items. If no one picks them up I'll likely take care of them eventually =)

@quicksnap
Copy link
Collaborator Author

quicksnap commented Mar 5, 2018

Opened #394 for that one.

@quicksnap
Copy link
Collaborator Author

Huh.. I failed the build but I'm not sure why..

@quicksnap
Copy link
Collaborator Author

That same command (npm run validate) passes on my machine.

@kentcdodds
Copy link
Contributor

My guess is there's a transitive dep issue.... May require a kcd-scripts update... I'll look into it when I find time.

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.

Ok, I've updated things and the build seems to be working now. Make sure to pull the latest changes on your branch before addressing the comments from @luke-john 👍 Thanks!

Copy link
Collaborator

@luke-john luke-john left a comment

Choose a reason for hiding this comment

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

Thanks again for adding this support.

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

Successfully merging this pull request may close these issues.

4 participants