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

Fixes #1666: Omit duplicated type in ColorFieldProps #2290

Merged
merged 1 commit into from Sep 7, 2021

Conversation

kwoncharles
Copy link
Contributor

Closes #1666

  • remove onChange prop of ValueBase

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

- remove `onChange` prop of ValueBase type
@@ -76,7 +76,7 @@ export interface Color {
formatChannelValue(channel: ColorChannel, locale: string): string
}

export interface ColorFieldProps extends ValueBase<string | Color>, InputBase, Validation, FocusableProps, TextInputBase, LabelableProps {
export interface ColorFieldProps extends Omit<ValueBase<string | Color>, 'onChange'>, InputBase, Validation, FocusableProps, TextInputBase, LabelableProps {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this get overridden below anyway? Do we need to explicitly omit 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.

@devongovett
You are right. However, when the --strict or --strictFunctionTypes option is enabled, Typescript complains about this as you can see in the issue.

It seems that the issue author's IDE is using the user-configured TS option rather than the one in react-spectrum/tsconfig.json.

If we don't need to consider it now, It's better to close it now :)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, in that case I'm fine with merging it.

@devongovett devongovett merged commit 48baaa1 into adobe:main Sep 7, 2021
@kwoncharles kwoncharles deleted the issues/1666 branch September 22, 2021 12:44
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.

Color Component failing on typecheck
2 participants