search

segmentio / evergreen

🌲 Evergreen React UI Framework by Segment
https://evergreen.segment.com
MIT License
12.36k stars 833 forks source link

Added type number to proptypes #1627

Open AbhiJain2196 opened 1 year ago

AbhiJain2196 commented 1 year ago

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

netlify[bot] commented 1 year ago

Deploy Preview for evergreen-storybook ready!

Name Link
Latest commit f923111471b8f2c546e7275c2b5c7185ae45c63f
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 commented 1 year ago

Thanks. Fixes #1625

ADTC commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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