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

Added type number to proptypes #1627

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AbhiJain2196
Copy link

@AbhiJain2196 AbhiJain2196 commented Apr 6, 2023

Overview
Fixes #1625
Instead of making changes in the SelectMenu.js file, we can directly change the SelectedPropType.js file and add PropTypes.number to PropTypes.oneOfType() function

Screenshots (if applicable)

Documentation

  • Updated Typescript types and/or component PropTypes
  • Added / modified component docs
  • Added / modified Storybook stories

@netlify
Copy link

netlify bot commented Apr 6, 2023

Deploy Preview for evergreen-storybook ready!

Name Link
🔨 Latest commit f923111
🔍 Latest deploy log https://app.netlify.com/sites/evergreen-storybook/deploys/642e8f6b9c3d5c0008d3816d
😎 Deploy Preview https://deploy-preview-1627--evergreen-storybook.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.

@ADTC
Copy link

ADTC commented Apr 6, 2023

Thanks. Fixes #1625

@ADTC
Copy link

ADTC commented Apr 6, 2023

It's not linking it for auto-closure in the sidebar. I think you'll have to edit the top description and add the Fixes phrase there yourself.

Thanks for the work. Appreciate it.

@ADTC
Copy link

ADTC commented Apr 6, 2023

Shouldn't we also support an array of numbers? I'm not sure why it allows an array of strings though, is that for multi-select?

But then again it does say multiple values are not supported atm so I don't know if it even matters or not.

@AbhiJain2196
Copy link
Author

In case of single or multi select, I think this code block will cover both the scenarios
PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, PropTypes.number]))

@ADTC
Copy link

ADTC commented Apr 6, 2023

In case of single or multi select, I think this code block will cover both the scenarios

PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, PropTypes.number]))

That won't work probably for single select, as it's not an array. This would:

PropTypes.oneOfType([
  PropTypes.string,
  PropTypes.number,
  PropTypes.arrayOf(PropTypes.oneOfType([
    PropTypes.string,
    PropTypes.number
  ]))
])

But that may not be proper, since it allows mixed arrays (both strings and numbers in the same array).

This below won't allow mixed arrays. The arrays have to be pure:

PropTypes.oneOfType([
  PropTypes.string,
  PropTypes.number,
  PropTypes.arrayOf(PropTypes.string),
  PropTypes.arrayOf(PropTypes.number)
])

I would go with this instead.

@brandongregoryscott
Copy link
Contributor

Hey folks - I think we'll need to update the TS definitions with whatever we use for PropTypes, as well. If you're using TS, you currently can't pass a number to selected either: https://codesandbox.io/s/pr-1627-selectmenu-number-typing-27seme?file=/src/App.tsx

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.

Property selected should include PropTypes.number to allow integer values
3 participants