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

Create custom width property for flyout #16

Merged
merged 1 commit into from Feb 20, 2018
Merged

Create custom width property for flyout #16

merged 1 commit into from Feb 20, 2018

Conversation

ghost
Copy link

@ghost ghost commented Feb 20, 2018

Sometimes design specifications could require the Flyout to extend pass the five standardized sizes. Because the flyout has no method for extending beyond the size, this PR adds a width property to the Flyout component in order to allow the flyout to have custom sizes for when the Flyout must be a different size other than the standardized sizes.

Pros

  • allows for flexibility in designing components that use the fly-out
  • avoids having to add extra sizes for larger fly-outs (i.e. xxl, xxxl, xxxxl)

Cons

  • less consistency in fly-outs if developers and engineers choose not to use the standardized sizes

custom flyout width screenshot

@chrislloyd
Copy link
Contributor

chrislloyd commented Feb 20, 2018

Hey @JediMasterRemington - this looks good. However, I think having both a width & a size property might be a little bit confusing. What would happen if somebody did the following?

<Flyout size="sm" width={800} ... />

Could you perhaps merge both properties together so that size also takes a number?

@ghost
Copy link
Author

ghost commented Feb 20, 2018

I've made it so size can now take a number

@@ -52,5 +52,5 @@ Flyout.propTypes = {
idealDirection: PropTypes.oneOf(['up', 'right', 'down', 'left']),
onDismiss: PropTypes.func.isRequired,
positionRelativeToAnchor: PropTypes.bool,
size: PropTypes.oneOf(['xs', 'sm', 'md', 'lg', 'xl']), // default: sm
size: PropTypes.oneOf(['xs', 'sm', 'md', 'lg', 'xl', PropTypes.number]), // default: sm
Copy link
Contributor

@chrislloyd chrislloyd Feb 20, 2018

Choose a reason for hiding this comment

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

I don't think this will do what you want it to. It's looking for the exact usage of PropTypes.number rather than

PropTypes.oneOfType(PropTypes.number, PropTypes.oneOf([ ... ]))

Copy link
Contributor

@chrislloyd chrislloyd left a comment

Choose a reason for hiding this comment

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

This is perfect! Thanks!

@chrislloyd chrislloyd merged commit 6f3c290 into pinterest:master Feb 20, 2018
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.

2 participants