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

RFC: transition and popup common shorthand #837

Closed
jeffcarbs opened this issue Nov 11, 2016 · 11 comments
Closed

RFC: transition and popup common shorthand #837

jeffcarbs opened this issue Nov 11, 2016 · 11 comments

Comments

@jeffcarbs
Copy link
Member

Background

I've been using <Popup> in my app for tooltips and it's a little cumbersome to use. Here's an example usage for showing a popup on a disabled button (note: this doesn't actually work out of the box since pointer-events are disabled on buttons 😐 )

const buttonElement = (
  <Button
    content='Some action!'
    disabled={disabled}
    loading={loading}
    onClick={onClick}
    primary
  />
)

if (!disabled) {
  return buttonElement
}

return (
  <Popup
    content='This button is disabled!'
    trigger={buttonElement}
  />
)

Proposal

What if we were to add a popup common shorthand prop to all components. The same example from above could be written as:

<Button
  content='Some action!'
  disabled={disabled}
  loading={loading}
  onClick={onClick}
  popup={disabled && 'This button is disabled!'}
  primary
/>

Under the hood, it would use a Popup.create shorthand factory:

Popup.create(props.popup, { trigger: /*regularly rendered button*/ })

// In this case would render
<Popup content='This button is disabled!' trigger={<Button>Some action</Button>} />

Transition

The Transition component (currently underway in #813) strikes me as something that will have the same cumbersome usage.

<Transition animation='slide'><Button /></Transition>

could be written as

<Button transition='slide' />

To me the former reads like "render a transition that results in a button", whereas the latter reads more naturally: "render a button with the slide transition".

Both the transition and popup props would be shorthand so they could be customized even further. For example:

<Button popup={{ inverted: true, on: 'click', content: 'Clicked!' }} />

Implementation

I think it would be crazy to try to do this with our components as-is given that we'd have to touch every single file. I really think we need to solve #419 first and get all of our components rendering via some common method. Whether it's a render method utility, a base component that every component eventually renders down to, or some factory.

cc @layershifter @levithomason - would love your thoughts!

@levithomason
Copy link
Member

Man, I love your proposals :) Yes, a popup prop has been on mind since day one. I also think we should solve the base component idea before doing this.

I hadn't considered transition, but it also makes perfect sense to me. This gets me thinking that the Dropdown menu could also take advantage of this pattern, using trigger. Really, any component wraps or appears beside another component could / should be added as a prop IMO.

Imagine:

<Dropdown trigger={<Image avatar src='...' />} options={options} />

// vs

<Image avatar src='...' dropdown={options} />

Not only is a single import needed, but it seems like it follows the more natural logical progression "I want to add a Popup/Dropdown to an Image" rather than "I want to wrap an Image in a Popup/Dropdown".

@levithomason
Copy link
Member

levithomason commented Dec 16, 2016

I've removed this from the 1.0 requirement, still think we should do this, just not as a blocking item.

@padrefuture
Copy link

Im looking into building a menu with an animated transition. Was thinking of using popup with a transition but it doesnt seem to work. Any ideas?

@layershifter
Copy link
Member

Transitions aren't supported by components yet. We're working on it, no ETA.

@padrefuture

This comment has been minimized.

@levithomason

This comment has been minimized.

@stale
Copy link

stale bot commented Mar 10, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 30 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 10, 2018
@stale stale bot closed this as completed Apr 9, 2018
@levithomason
Copy link
Member

Thanks to @rijk, we have improved how we handle stale issues, see #2761. Reopening.

@levithomason levithomason reopened this May 14, 2018
@stale stale bot removed the stale label May 14, 2018
@pferreir
Copy link
Contributor

Any news on this? Any way I could help?

@stale
Copy link

stale bot commented Nov 25, 2018

There has been no activity in this thread for 180 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 180 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

@stale stale bot added the stale label Nov 25, 2018
@stale
Copy link

stale bot commented May 24, 2019

This issue will be closed due to lack of activity for 12 months. If you’d like this to be reopened, just leave a comment; we do monitor them!

@stale stale bot closed this as completed May 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants