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

Feature: Add closeOnSelect & onSelect to Navbar API with tests & docs #2280

Merged
merged 13 commits into from
Oct 27, 2016

Conversation

kevinzwhuang
Copy link
Member

@kevinzwhuang kevinzwhuang commented Oct 12, 2016

Update 10/28: closeOnSelect has been renamed to collapseOnSelect to fit better semantically, given that it changes on the expanded prop and not open.
Update 10/27:
Several improvements have been made on this API since the last update on 10/17.
The demo of the latest build can be run here: https://plnkr.co/edit/SNvGZyLkiBmWOnyT86yG
A summary of what it looks like now is as follows:

onSelect: React.PropTypes.func
A callback fired when a descendant of a child <Nav> is selected. Should
be used to execute complex closing or other miscellaneous actions desired
after selecting a descendant of <Nav>. Does nothing if no <Nav> or <Nav>
descendants exist. The callback is called with an eventKey, which is a
prop from the selected <Nav> descendant, and an event.

function (
  Any eventKey,
  SyntheticEvent event?
)

For basic closing behavior after all <Nav> descendant onSelect events in
mobile viewports, try using closeOnSelect.

Note: If you are manually closing the navbar using this OnSelect prop,
ensure that you are setting expanded to false and not toggling between
true and false.


closeOnSelect: React.PropTypes.bool

Sets expanded to false after the onSelect event of a descendant of a
child <Nav>. Does nothing if no <Nav> or <Nav> descendants exist.

The onSelect callback should be used instead for more complex operations
that need to be executed after the select event of <Nav> descendants.


Update 10/17:
This PR has been improved from its original implementation. To see a running demo of the current iteration, you can run this Plunker here: https://plnkr.co/edit/43i4XA

The proposal has been simplified and improved with these two new props to the <Navbar> API:

onSelect: React.PropTypes.func
A callback fired when a <NavItem> grandchild is selected inside a child
<Nav>. Should be used to execute complex toggling or other miscellaneous
actions desired after selecting a <NavItem>. Does nothing if no <Nav>
& <NavItem> children exist. The callback is called with an eventKey,
which is a prop from the <NavItem>, and an event.

function (
   Any eventKey,
   SyntheticEvent event?
)

For basic toggling after all <NavItem> select events, try using
toggleOnSelect.


toggleOnSelect: React.PropTypes.bool

Fires the onToggle callback whenever a <NavItem> inside the child <Nav> is
selected. Does nothing if no <Nav> & <NavItem> children exist.

The onSelect callback should be used instead for more complex operations
desired for after <NavItem> select events.

/Update


Description:

Auto toggling as a behavior in react-bootstrap has been discussed at length in #1301 & #1692. @jquense and @taion have made good points against implementing it into the API due to complexities of different cases. There have also been valid arguments made for a simple version of this behavior to have available for users that does not need to catch every edge case. I think there is value to having a simple implementation of this expected behavior (normal click menus) for many users.

After reading over the discussion, I'd like to propose a simple addition to the Navbar API - lazyAutoToggle. I thought of naming it lazy to make it very apparent that it goes about autotoggling in a nondiscriminant & noncomplex way on <Navbar.Collapse> and its subcomponents. It does not attempt to be an omni-solution for all toggling needs.

Demo:

I've written a demo of this functionality in the plnkr. You can run the example at the link below:
https://plnkr.co/edit/vDejOB

There you can compare the functionality of a <Navbar> with & without lazyAutoToggle on.
It uses the built version of react-bootstrap from this PR.

How to use:

Simply add lazyAutoToggle to a <Navbar> with a child <Navbar.Collapse>.

Changes Proposed:

  • Add lazyAutoToggle prop to Navbar
  • Add lazyAutoToggle to childContext of Navbar
  • Check for lazyAutoToggle and onToggle from this.context.$bs_navbar in <Navbar.Collapse> to fire onToggle when both exist

This solution is fairly straightforward. It involves adding the prop value of lazyAutoToggle to the childContext of <Navbar> and its children - particularly for use by <Navbar.Collapse>.

<Navbar.Collapse> will then fire onToggle from its parent <Navbar> if and only if lazyAutoToggle && onToggle exist in this.context.$bs_navbar.

To support this new prop, I've also added:

  • Test for lazyAutoToggle on Navbar.Collapse clicks
  • Test for lazyAutoToggle on Navbar.Collapse subcomponent click + onClick events
  • Documentation for lazyAutoToggle in Navbar propTypes.

Not included:

Since this is a simple solution, this will not attempt to cover any complex cases where individual items might need to prevent toggling. I can foresee a roadmap towards providing an array prop of components that should be ignored by lazyAutoToggle, but that is not a part of this PR.

As a single prop solution, lazyAutoToggle works extremely well with regular links, buttons, etc. (normal click-related menu items).


cc: @jquense and @taion

@taion
Copy link
Member

taion commented Oct 12, 2016

I don't think this is the right approach. I think you should hook into onSelect in <Nav>.

@kevinzwhuang
Copy link
Member Author

Hooking into onSelect in <Nav> is a good idea, I'll look into that later today.

@codecov-io
Copy link

codecov-io commented Oct 14, 2016

Current coverage is 93.34% (diff: 100%)

Merging #2280 into master will increase coverage by <.01%

@@             master      #2280   diff @@
==========================================
  Files            94         94          
  Lines          1683       1684     +1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1571       1572     +1   
  Misses          112        112          
  Partials          0          0          

Powered by Codecov. Last update 61fe9e1...861e154

@kevinzwhuang
Copy link
Member Author

You were totally right @taion, the hook on <Nav> is a much better approach. Overall, a way simpler strategy compared to the <Navbar.Collapse> idea and needs only 3 lines of code in Nav.js

I also updated the docs & test for this approach in the latest commit.
Here is an updated working demo in Plunker for you to run in your browser: https://plnkr.co/edit/AYyb2G

@@ -93,6 +93,8 @@ const defaultProps = {
const contextTypes = {
$bs_navbar: React.PropTypes.shape({
bsClass: React.PropTypes.string,
lazyAutoToggle: React.PropTypes.bool,
Copy link
Member

Choose a reason for hiding this comment

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

i'd just pass down an onSelect here

* Does nothing if no child <Nav> is present. lazyAutoToggle should not be used
* for complex nav menus.
*/
lazyAutoToggle: React.PropTypes.bool,
Copy link
Member

Choose a reason for hiding this comment

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

I'd call this toggleOnSelect (or maybe autoToggle)

@kevinzwhuang
Copy link
Member Author

toggleOnSelect sounds good with me. Could you clarify a bit more on the onSelect, it doesn't seem $bs_navbar has an onSelect set from <NavBar>'s getChildContext()

@taion
Copy link
Member

taion commented Oct 15, 2016

@kevinzwhuang I mean add one. @jquense wdyt?

@kevinzwhuang
Copy link
Member Author

kevinzwhuang commented Oct 15, 2016

@taion ah I see what you mean. 👍

In that case we might not even need a toggleOnSelect/autoToggle flag, since the exposed onSelect hook into <Nav>'s chained onSelect function provides even more flexibility and allows users to do the exact same autoToggle functionality themselves by passing in the same callback for onToggle for the onSelect context (or whatever modified toggle function they want). No need for prescribing behavior into the API and lets users handle their own edge cases!

Though I think maybe it should be named onNavSelect or onNavSelectCompleted to denote that it's a hook into <Nav>'s onSelect chain and not just an onSelect callback meant for <NavBar> itself (which is what I thought you meant at first)

@taion
Copy link
Member

taion commented Oct 15, 2016

Personally I think onSelect is fine. I think it's pretty clear in context that onSelect on a <Navbar> would refer to the <NavItem>s under it.

I feel like this "toggle on select" behavior you're describing is common enough that it'd make sense to have as an option. It may not account for all edge cases, but it's probably what most users want.

@kevinzwhuang
Copy link
Member Author

Sounds good @taion. I'll work on updating this PR with that approach sometime this weekend.

@kevinzwhuang kevinzwhuang changed the title Feature: Add lazyAutoToggle to Navbar with tests & docs Feature: Add toggleOnSelect & onSelect to Navbar API with tests & docs Oct 17, 2016
@kevinzwhuang
Copy link
Member Author

Updated to the latest changes as discussed with toggleOnSelect & onSelect. I made it so that when an onSelect callback is provided, toggleOnSelect will be ignored (makes sense if you view onSelect as a more complex option to toggleOnSelect).

Docs & tests have been updated to reflect this new API.

Also I've created an updated Plunker with the latest build of this PR. Works well with NavItems & NavDropdown/MenuItems mixed in. (I believe NavDropdown's are ignored by toggleOnSelect & onSelect due to it not having a select event inherently).

In this latest demo, I've added a particular example using the onSelect callback to skip toggling for eventKey 2 - the potential for this callback is promising given it gives users access to the eventKey + event. You can try it out here: https://plnkr.co/edit/cOc8ZI

Let me know what you think @taion @jquense

@@ -319,6 +322,7 @@ class Nav extends React.Component {
const childOnSelect = createChainedFunction(
child.props.onSelect,
onSelect,
navbar && (navbar.onSelect || navbar.toggleOnSelect && navbar.onToggle) || null,
Copy link
Member

Choose a reason for hiding this comment

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

can you put all this logic in e.g. a handleSelect on <Navbar>?

@kevinzwhuang
Copy link
Member Author

Folded the navbar && (navbar.onSelect || navbar.toggleOnSelect && navbar.onToggle) || null logic into handleSelect in <Navbar>.

Tests did not need to be modified since it's the same behaviour, updated plunker with latest build continues to work, which you can run here: https://plnkr.co/edit/PZhOUx

@@ -100,6 +126,7 @@ class Navbar extends React.Component {
bsClass,
expanded,
onToggle: this.handleToggle,
onSelect: this.handleSelect,
Copy link
Member

Choose a reason for hiding this comment

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

sorry, one last thing – i think it might be better to make this just

createChainedFunction(onSelect, toggleOnSelect && this.handleToggle)

I think it's pretty unlikely that users will specify both toggleOnSelect and onSelect, and I think it'd be a bit cleaner this way.

Otherwise, this is looking really good. Thanks!

@taion
Copy link
Member

taion commented Oct 17, 2016

Thanks!

@kevinzwhuang
Copy link
Member Author

Thanks for your guidance on this @taion! Any more thoughts on this @jquense, @taion?

Copy link
Member

@jquense jquense left a comment

Choose a reason for hiding this comment

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

navbar-btn Buttons aren't going to work correctly with this. it doesn't fail, but it may be unexpected that they don't work? do we care?


return {
$bs_navbar: {
bsClass,
expanded,
onToggle: this.handleToggle,
onSelect: createChainedFunction(onSelect, toggleOnSelect && this.handleToggle || null),
Copy link
Member

Choose a reason for hiding this comment

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

should this actually toggle or only close the navbar? Theoretically you can't select something if its collapsed, but double clicks may get through, or other weird races.

Copy link
Member Author

Choose a reason for hiding this comment

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

It should toggle, only after the NavBar's onSelect if it exists and the Nav/NavItem's onSelect (the sequence is on Nav.js line 320). The race with double clicks is inevitable with the current API, since it happens with Navbar.Toggle already currently.

});

it('Should fire onSelect with eventKey for nav children', () => {
const selectSpy = sinon.spy();
Copy link
Member

Choose a reason for hiding this comment

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

does this work correctly with a NavDropdown?

Copy link
Member Author

@kevinzwhuang kevinzwhuang Oct 26, 2016

Choose a reason for hiding this comment

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

NavDropdown's are ignored by onSelect & toggleOnSelect and their subcomponent MenuItems fire those callbacks correctly. I think this is the correct behavior out of the box - you can see it demonstrated here:
https://plnkr.co/edit/4sn165?p=preview (when you click the NavDropdown, no onSelect or onToggle callbacks are fired for either configuration).

@kevinzwhuang
Copy link
Member Author

kevinzwhuang commented Oct 26, 2016

I added a condition for toggleOnSelect to fire this.handleToggle only if window is not undefined(i.e. within a node build/test env) && window.innerWidth < 768 in Navbar.js, since it doesn't make sense for it to toggle when in a viewport larger than a mobile screen. Hope that makes sense @taion @jquense

Updated plunker with this build version: https://plnkr.co/edit/4sn165?p=preview


Update: Changed the condition to a function that evaluates innerWidth on runtime(whereas before it only being evaluated when getChildContext was run. This seems to be the better way to do it, so it can handle changes in browser width. See b9e0895. The plunker has also been updated with this new build.


Update on this: Checking innerWidth has been dropped an approach to handling different viewports. Rather, simply setting the state of expanded to false to close the Navbar is more important than toggling. View this discussion for the newest approach: #2280 (comment)

const { bsClass, expanded } = this.props;
const { bsClass, expanded, onSelect, toggleOnSelect } = this.props;
const mobileToggle = () => {
if (typeof(window) !== 'undefined' && window.innerWidth < 768) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this check. it's too brittle anyway as a user can change the navbar collapse breakpoint to whatever they want so hard coding a pixel width here isn't ideal.

I do think that it illustrates a more general problem with this feature tho, you don't really want to be toggling expanded state while the expansion is not relevant, however there isn't really any way to know that from JS

Copy link
Member Author

@kevinzwhuang kevinzwhuang Oct 26, 2016

Choose a reason for hiding this comment

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

Yeah it's brittle hard-coded as is.

One approach I can think of dealing with is is exposing a prop toggleOnSelectMaxWidth. It's of React.PropTypes.number and defaults to 768. So then when toggleOnSelect == true the window.innerWidth check would compare with toggleOnSelectMaxWidth.

Or toggleOnSelect could be the numeric value itself, and default to 0, so it's falsey when not set. When set, it can serve as the maxWidth itself.

Copy link
Member

Choose a reason for hiding this comment

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

Does anything bad happen if we unnecessarily toggle collapsed state? The styling is a no-op on desktop views, plus the user has to opt-in to this behavior anyway.

Copy link
Member Author

@kevinzwhuang kevinzwhuang Oct 26, 2016

Choose a reason for hiding this comment

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

The whole navbar looks like it sort of re-renders when it tries to collapse while in a non-mobile viewport.

Here's an older plunker that demonstrates this: https://plnkr.co/edit/PZhOUx?p=preview
(make sure to resize the iframe to greater than 768px)

If we can figure out a way to fix this behavior in the lifecycle of the navbar, then we could get away with not having to deal with a arbitrary innerWidth breakpoint.

Copy link
Member

Choose a reason for hiding this comment

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

I'm really against users needing to input there grid breakpoint in order for this to work well. IMO it defeats the value of having this "add on" functionality if is comes with config, or a gotcha.

The only way i can think of tho to handle this is some sort of computedStyle sniffing to detect if the nav is in the right media query...that's not much less brittle tho :/

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree - users shouldn't have to set a breakpoint.

I think I found a solution to this problem. It goes back to the point you mentioned about toggling vs closing the navbar! We were tackling it from the wrong angle with toggleOnSelect, which is causing the navbar to toggle between expanded states even when the screen size is not appropriate. In fact, screen size should not matter - only the value of expanded should really matter.

The right functionality should be something more like a closeOnSelect or collapseOnSelect, which will set expanded to false, only if it was true. If expanded is false, then it does nothing. I'll update this PR with a commit to demonstrate this new approach shortly.

@taion
Copy link
Member

taion commented Oct 26, 2016

@jquense I don't care about Navbar buttons. How do those even show up on mobile/collapsed anyway? IMO it's a non-problem.

@taion
Copy link
Member

taion commented Oct 27, 2016

That's a great idea. closeOnSelect sounds better, too. 😄

@kevinzwhuang
Copy link
Member Author

kevinzwhuang commented Oct 27, 2016

Latest commit adds closeOnSelect. Works as expected :)
Here's a demo of the new build: https://plnkr.co/edit/SNvGZyLkiBmWOnyT86yG

@kevinzwhuang kevinzwhuang changed the title Feature: Add toggleOnSelect & onSelect to Navbar API with tests & docs Feature: Add closeOnSelect & onSelect to Navbar API with tests & docs Oct 27, 2016
Copy link
Member

@taion taion left a comment

Choose a reason for hiding this comment

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

We can deal with the comment nits later

@@ -57,7 +58,36 @@ const propTypes = {
* @controllable navExpanded
*/
onToggle: React.PropTypes.func,

/**
* A callback fired when a `<NavItem>` grandchild is selected inside a child
Copy link
Member

Choose a reason for hiding this comment

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

"descendant" rather than "grandchild" (since it's actually Navbar > Navbar.Collapse > Nav > NavItem, and there might even be a LinkContainer or so in there)

Copy link
Member Author

Choose a reason for hiding this comment

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

What about "...when a descendant of a child <Nav> is selected..."? This might refer to NavItem, LinkContainer, etc., basically any descendants that can be selected without referring to any specific ones.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds great.

/**
* Sets `expanded` to false after the onSelect event of a child of `<Nav>`
* (such as `<NavItem>` or `<MenuItem>`). Does nothing if no `<Nav>` or
* `<NavItem>`/`<MenuItem>` children exist.
Copy link
Member

Choose a reason for hiding this comment

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

<MenuItem> only goes under dropdowns actually

Copy link
Member Author

Choose a reason for hiding this comment

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

Working off of what I brainstormed in the comment above, how's this?
"
Sets expanded to false after the onSelect event of a descendant of a child <Nav>. Does nothing if no <Nav> or <Nav> descendants exist.
"

Copy link
Member

Choose a reason for hiding this comment

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

👍

@jquense
Copy link
Member

jquense commented Oct 27, 2016

looks good to me. Thanks for the great work. Let me know @kevinzwhuang if you've like to be part of the react-bootstrap organization*. Its really low commitment, contribute as you have time.

(baring unlikely disagreement from other @react-bootstrap/collaborators)

@kevinzwhuang
Copy link
Member Author

Just updated the PR with those comment improvements discussed.

@taion @jquense Thank you for the help on this. I'd be glad to be a part of the org and contribute. 😄

@GerardoMachadoGandaria
Copy link

Does not seem to work on mobile device?

@kevinzwhuang
Copy link
Member Author

@GerardoMachadoGandaria could you provide some more details? I use this in my own apps and just tested this again myself on Android and iPhone and there's no problem.

Are you using react-bootstrap@0.30.6? What device are you using? Are you using the closeOnSelect or collapseOnSelect prop? The API has since been renamed to collapseOnSelect in #2308 - there's also a note about this at the top of this PR, so using closeOnSelect will be the incorrect prop. If you have some code you could share that would be great.

@GerardoMachadoGandaria
Copy link

Sorry my fault. Works fine.
Something was probably buffering an old version of the app.
Restart of the server did the trick. Also. Thank you for this change!

@kevinzwhuang
Copy link
Member Author

Glad it worked out 😄

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.

5 participants