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

Updated to csstype@3 #38

Merged
merged 8 commits into from
Feb 22, 2022

Conversation

rahulsunil2
Copy link
Contributor

@rahulsunil2 rahulsunil2 commented Feb 19, 2022

Closing Issue #31 Update to csstype@3

  • Updated CSS Type to 3.0.10

@rahulsunil2 rahulsunil2 requested a review from a team as a code owner February 19, 2022 19:08
@ghost
Copy link

ghost commented Feb 19, 2022

CLA assistant check
All CLA requirements met.

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

Thank you, that really helps ❤️


In meantime I added proper type checking to this repo (#42) and it discovered few problems:

Error: ../core/src/types.ts(59,65): error TS2694: Namespace '"/home/runner/work/griffel/griffel/node_modules/csstype/index"' has no exported member 'AnimationProperty'.

This is a problem indeed, just perform that same change:

// packages/core/src/types.ts
export type GriffelStylesStrictCSSObject = GriffelStylesCSSProperties &
  GriffelStylesCSSPseudos & {
-    animationName?: GriffelAnimation | GriffelAnimation[] | CSS.AnimationProperty;
+    animationName?: GriffelAnimation | GriffelAnimation[] | CSS.Property.AnimationName;
Error: src/types.test.ts(86,5): error TS2578: Unused '@ts-expect-error' directive.
Error: src/types.test.ts(98,9): error TS2578: Unused '@ts-expect-error' directive.
Error: src/types.test.ts(112,9): error TS2578: Unused '@ts-expect-error' directive.
Error: src/types.test.ts(127,11): error TS2578: Unused '@ts-expect-error' directive.

These are in tests, please update all of them in this way:

-// @ts-expect-error "1" is invalid value for "flexShrink"
-flexShrink: '1',
+// @ts-expect-error "1" is invalid value for "overflow"
+overflow: '1',

@layershifter
Copy link
Member

I checked these changes in Fluent UI repo and everything builds:

image

Once comments will be addressed we are good to go 🏍

@rahulsunil2
Copy link
Contributor Author

I'll fix the issues mentioned in the comments as soon as possible and request a new review.

@rahulsunil2
Copy link
Contributor Author

@layershifter Sorry, I have no experience writing tests in js but I would love to learn how to. Can you please tell me what I should do and provide me with some reference material? I will work on it at the earliest.

@layershifter
Copy link
Member

@layershifter Sorry, I have no experience writing tests in js but I would love to learn how to. Can you please tell me what I should do and provide me with some reference material? I will work on it at the earliest.

@rahulsunil2 in the previous message I meant to update these usages as was suggested:

// @ts-expect-error "1" is invalid value for "flexShrink"
assertType({ flexShrink: '1' });

-// @ts-expect-error "1" is invalid value for "flexShrink"
-flexShrink: '1',
+// @ts-expect-error "1" is invalid value for "overflow"
+overflow: '1',

There is no need to write new ones 😃

@rahulsunil2
Copy link
Contributor Author

I have made the fixes for both types.test.ts and types.ts.

@rahulsunil2
Copy link
Contributor Author

rahulsunil2 commented Feb 21, 2022

There is an Expression produces a union type that is too complex to represent. error in generateStyles.ts file.

image

Can you please confirm, whether this is caused due to my updates?

@rahulsunil2
Copy link
Contributor Author

Thanks @layershifter, I will go through the solution. 👍

@layershifter
Copy link
Member

There is an Expression produces a union type that is too complex to represent. error in generateStyles.ts file.

image

Can you please confirm, whether this is caused due to my updates?

@rahulsunil2 it's related and not related 😸

  • Related because it really breaks 💥 (and it's kinda understandable from TypeScript POV)
  • Not related because typings on generateStyles() function were not ideal even before 😅

I pushed a minor refactor in 8432f3f and now CI is passing ✅

@@ -2,7 +2,7 @@ import * as CSS from 'csstype';

export type GriffelStylesCSSValue = string | 0;

type GriffeltylesUnsupportedCSSProperties = {
Copy link
Member

Choose a reason for hiding this comment

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

These was a typo before. As type was not exported, we can fix it 😇

@layershifter layershifter merged commit e62b24f into microsoft:main Feb 22, 2022
@layershifter layershifter mentioned this pull request Feb 22, 2022
@rahulsunil2
Copy link
Contributor Author

Thanks a lot @layershifter for explaining and letting me contribute to this awesome repo! I hope I could contribute more. Before I really need to understand a lot more about TS. 😄

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.

None yet

3 participants