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.

Sign up for GitHub

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

Jump to bottom

chore(project): add initial typescript setup and config #11533

Closed
wants to merge 5 commits into from
Closed

chore(project): add initial typescript setup and config #11533

wants to merge 5 commits into from

Conversation

joshblack
Copy link
Contributor

@joshblack joshblack commented Jun 2, 2022 •
edited
Loading

Closes #11317

This PR creates an initial TypeScript setup within the project for us to experiment with for the @carbon/icons-react package. It also includes an initial implementation of the types for this package based on what exists in DefinitelyTyped.

Changelog

New

  • Add initial TypeScript configuration to the project and initial CI workflow
  • Add Icon.d.ts to the icons-react package

Changed

  • Update icon-build-helpers to emit a root index.d.ts file in a package
  • Update icons-react to specify typings file and include in published package

Removed

Testing / Reviewing

  • Go over TypeScript configuration to see if it:
    • Makes sense for our monorepo setup
    • Has relevant compiler option rules enabled
    • Is being correctly used by our typecheck command to identify type errors

@netlify
Copy link

netlify bot commented Jun 2, 2022 •
edited
Loading

✅ Deploy Preview for carbon-components-react ready!

Name Link
🔨 Latest commit be349e1
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/6298f6de28e40c00096da298
😎 Deploy Preview https://deploy-preview-11533--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Jun 2, 2022 •
edited
Loading

✅ Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit be349e1
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6298f6de7adf790009da697d
😎 Deploy Preview https://deploy-preview-11533--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@joshblack joshblack marked this pull request as ready for review June 2, 2022 17:40
@joshblack joshblack requested a review from a team as a code owner June 2, 2022 17:40
SimeonC
SimeonC reviewed Jun 7, 2022
View reviewed changes
packages/icons-react/types/Icon.d.ts

import * as React from 'react';

type IconSize = 16 | 20 | 24 | 32 | '16' | '20' | '24' | '32' | number | string;
Copy link

Choose a reason for hiding this comment

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

If this is changed to ... | '32' | (number & {}) | (string & {}) then autocomplete in IDE's will prefer the actual values rather than a generic string/number but still won't force it. See microsoft/TypeScript#29729 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@joshblack Are there numbers outside of 16, 20, 24, and 32 that an Icon can use?

@jharvey10 jharvey10 mentioned this pull request Jun 17, 2022
Closed
2 tasks
@joshblack joshblack closed this Jul 19, 2022
@metonym
Copy link
Collaborator

metonym commented Aug 5, 2022

@types/carbon__icons-react has been updated to v11.5.0. The DefinitelyTyped maintainers were kind enough to merge the updated Carbon icons types.

Are there plans to revive this PR? One of the maintainers recommended generating a single index.d.ts, which is not dissimilar to the approach in this PR.

If type definitions are shipped with the package, @types/carbon__icons-react can be removed for good.

@metonym metonym mentioned this pull request Aug 5, 2022
Merged
8 tasks
@peterblazejewicz
Copy link

There was another suggestion on DT, that could be even better, using inline JSDoc comments (@type and the family), so there no work (no artefacts build steps) required for TS support for all of those symbols, thx!
DefinitelyTyped/DefinitelyTyped#52664 (comment)

@metonym
Copy link
Collaborator

metonym commented Aug 30, 2022 •
edited
Loading

@tay1orjones Given the renewed TypeScript effort, could this PR be revived and merged?

I think this PR gets the best of both worlds because it fully supports TS without needing to rewrite the package in TS. It appears to carry low risk while solving the need for typing this high-demand package.

EDIT: To avoid spamming this thread, I copied this branch and submitted it as a new PR: #12034

@metonym metonym mentioned this pull request Aug 30, 2022
Closed
@tay1orjones tay1orjones mentioned this pull request Oct 20, 2022
Merged
kennylam pushed a commit to kennylam/carbon that referenced this pull request Jul 30, 2024
@ariellalgilmore
feat(modal): ai updates (carbon-design-system#11533)
887ef40 
### Related Ticket(s)

Closes carbon-design-system#11500 

### Description

updates modal to latest AI gradient updates
### Changelog

**New**

- modal now checks if footer slotted for styles

**Changed**

- modal-footer reverted code back to only checking if there are three buttons
- updated styles

**Removed**

- {{removed thing}}

<!-- React and Web Component deploy previews are enabled by default. -->
<!-- To enable additional available deploy previews, apply the following -->
<!-- labels for the corresponding package: -->
<!-- *** "test: e2e": Codesandbox examples and e2e integration tests -->
<!-- *** "package: services": Services -->
<!-- *** "package: utilities": Utilities -->
<!-- *** "RTL": React / Web Components (RTL) -->
<!-- *** "feature flag": React / Web Components (experimental) -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SimeonC SimeonC SimeonC left review comments

@jharvey10 jharvey10 jharvey10 left review comments

@tay1orjones tay1orjones tay1orjones approved these changes

@jnm2377 jnm2377 jnm2377 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Design System
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: @carbon/react/icons module does not provide IDE import auto-complete
7 participants
@joshblack @metonym @peterblazejewicz @SimeonC @tay1orjones @jnm2377 @jharvey10