Skip to content

Conversation

taion
Copy link
Member

@taion taion commented Sep 15, 2015

It's painful and annoying to have to make a custom Link component for every component we want to use.

This replaces everything with a LinkContainer component that can wrap any React-Bootstrap component (and potentially plenty of others as well).

This is also targeted against React Router v1.0.0-rc1.

As part of this, I've stripped out a number of the visual tests that were trivial/pointless (pagination and thumbnails) or wrong (the dropdown, where normal menu items ignore the injected onClick prop).

@taion
Copy link
Member Author

taion commented Sep 15, 2015

My intent would be to release this as v0.19.0. This will allow us to close almost all the current open issues.

@AlexKVal
Copy link
Member

Very nice 👍
You've gutted it, indeed 😄

@jquense
Copy link
Member

jquense commented Sep 15, 2015

I love this. one thought. maybe we add a trigger prop that defaults to onClick? then the user can wrap stuff that triggers other events? cough menuitem

@taion
Copy link
Member Author

taion commented Sep 15, 2015

You mean for e.g. onSelect? I looked at that a little bit but the signature doesn't quite match, since this would ignore the eventKey.

Do you think it makes more sense to do that than to just have MenuItem call its props.onClick?

@mtscout6
Copy link
Member

This looks really great! Should we consider renaming the project now that it's not specific to react-bootstrap?

@taion
Copy link
Member Author

taion commented Sep 15, 2015

That's where I started - it's a little bit specific to React-Bootstrap, though. The active and disabled props really match the signature of our specific components. They're somewhat generic, but not 100%.

@mtscout6
Copy link
Member

Gotcha!

@jquense
Copy link
Member

jquense commented Sep 15, 2015

Do you think it makes more sense to do that than to just have MenuItem call its props.onClick?

I do, I was just thinking a little of both. More important to just fix the MenuItem tho. To the signature thing onSelect should provide the event first, so the preventDefault would work in that specific case. I know b/c thats what we are doing as of like 3 days ago :P

@taion
Copy link
Member Author

taion commented Sep 15, 2015

I'm a little hesitant to do that, though, because the handleClick method from Link specifically calls into props.onClick, and I'd rather not copy-paste that entire method: https://github.com/rackt/react-router/blob/d88a051516d53eeb5c3fceb8f7ad0df325b3f404/modules/Link.js#L67-L68

I think I'd rather we just fix MenuItem.

Are we okay with releasing this?

@jquense
Copy link
Member

jquense commented Sep 15, 2015

LGTM

for reference tho I meant something more like:

  handleEvent(event = {}) {
    var allowTransition = true

    if (event.type === 'click' && isModifiedEvent(event) || !isLeftClickEvent(event))
      return

    if (event.defaultPrevented === true)
      allowTransition = false

    event.preventDefault()

    if (allowTransition)
      this.context.history.pushState(this.props.state, this.props.to, this.props.query)
  },

  render() {
    var { history } = this.context
    var { children, activeClassName, trigger, activeStyle, onlyActiveOnIndex, to, query, state, ...props } = this.props

    ;[].concat(trigger).forEach(eventName => {
      props[eventName] = chain(props[eventName], this.handleEvent)
    })

@taion
Copy link
Member Author

taion commented Sep 15, 2015

Makes sense - could come in handy in case those use cases come up.

taion added a commit that referenced this pull request Sep 15, 2015
[changed] Replace everything with LinkContainer
@taion taion merged commit 5851466 into react-bootstrap:master Sep 15, 2015
@taion taion deleted the next branch September 15, 2015 17:32
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.

4 participants