preventDefault specified to work only for disabled && onSelect; it is… #2790

jackpetraitis · 2017-09-12T16:00:01Z

… a fix for issue #2711

taion · 2017-09-12T16:19:35Z

jackpetraitis · 2017-09-12T16:45:38Z

Oh, @taion I've been meaning to ask you why. Why would we want to drop it across all onSelect?
Just wondering. But also, I found the related behavior in these items:

NavItem
Panel
PanelGroup

I will update my PR with a fix for those as well. I saw some mention of this.props.onSelect in Carousel too, but I don't think it applies here.

jquense · 2017-09-12T16:49:40Z

thanks! the reason why is that it ends up being more harm than help to prevent the link behavior. There are a few places where it still makes sense but I'm not sure which places.

ALSO please make the PR against the next branch not master as this is a breaking change

taion · 2017-09-12T19:12:23Z

You need to rebase this branch on next too.

We want to change it everywhere because we want all components here to offer similar APIs. It would be confusing if onSelect on different components behaved differently.

jackpetraitis · 2017-09-12T19:24:08Z

Thanks. I'm currently rebasing this branch onto next in my fork. Then I will re-open a PR with a request to merge my next into react-bootstrap's next.

taion · 2017-09-12T19:48:03Z

You need to run git rebase --interactive next on your branch, and restrict the branch to only your own commits.

jackpetraitis · 2017-09-12T21:01:32Z

So I had done an interactive rebase before on this. I think that's what messed it up.

I had done my changes off master. And then I rebased master onto next. Then I force pushed my next branch up to my forked repo. And now here we are. I'm wondering why all these other people's changes are still showing up as part of this PR. Maybe you could direct me as to how I can restrict the next branch to only my own commits. Perhaps you meant I should be creating a feature branch for just my own commits and then doing a PR to next with that feature branch?

Let me know if you can decipher what I may have done wrong from above.
I really only changed:

NavItem.js
NavBarSpec.js

Panel, PanelGroup, and their respective spec files already got refactored with the latest from next, so I think their preventDefault issue is no longer an issue. Although their changes are still reflected in this PR.

So let me fix the PR and sort this out properly. I will try this out in a few hours.

jackpetraitis · 2017-09-13T02:11:59Z

Found the behavior in more items:

DropdownButtonCustomMenu
NavDropdown
SubNav
Dropdown
DropdownMenu
MenuItem
Nav
PagerItem
SafeAnchor
SafeAnchorSpec

I'm going to investigate each of these and drop the preventDefault if possible.
And testing each of the components rendered in a browser.

… a fix for issue react-bootstrap#2711

… and PanelGroup

…ntDefault is clicked and for whether the navbar collapses when it is clicked.

taion · 2017-09-13T03:38:54Z

I rebased and force pushed to your branch. You'll want to run something like:

$ git fetch
$ git reset --hard origin/master

assuming you don't have any other work you want to preserve on your current master branch.

It turns out that due to #1769, we don't need this for <Panel> and related components any more.

The only one left is actually just <PagerItem>, as far as I can tell.

taion · 2017-09-13T03:39:12Z

src/NavItem.js

@@ -28,12 +28,8 @@ class NavItem extends React.Component {
  }

  handleClick(e) {
-    if (this.props.onSelect) {
-      e.preventDefault();


we probably still want to do e.preventDefault() if disabled is set?

taion · 2017-09-13T03:39:30Z

test/NavbarSpec.js

+    ReactTestUtils.Simulate.click(ReactDOM.findDOMNode(link));
+
+    expect(navItemSpy.getCall(0).args[0].isDefaultPrevented()).to.be.false;
+  })


missing semi here is causing lint failure

jackpetraitis · 2017-09-13T14:06:27Z

My preventDefault's snooping gave me the same result you found @taion

DropdownButtonCustomMenu <- don’t need as this was an example
NavDropdown <- also an example
SubNav <- in the docs
Dropdown <- seems to be an important part of the handleKeyDown logic
DropdownMenu <- same, has a role to play in handleKeyDown
MenuItem <- this can stay here
Nav <- part of handleTabKeyDown logic
PagerItem <- changed to preventDefault only if PagerItem is disabled && onSelect
SafeAnchor <- this can also stay the same
SafeAnchorSpec <- don’t need to change

… onSelect. also modifying code so it only will preventDefault if element is disabled

jquense · 2017-09-13T14:14:17Z

src/NavItem.js

-    if (this.props.onSelect) {
+    if (this.props.onSelect && !this.props.disabled) {
+      this.props.onSelect(this.props.eventKey, e);
+    } else if (this.props.disabled) {


you can rewrite this a bit simpler as

if (this.props.disabled) { e.preventDefault(); return; } if (this.props.onSelect) this.props.onSelect(this.props.eventKey, e);

I am for sure a fan of simpler.

jquense · 2017-09-13T14:15:02Z

src/PagerItem.js

@@ -30,7 +30,7 @@ class PagerItem extends React.Component {
  handleSelect(e) {
    const { disabled, onSelect, eventKey } = this.props;

-    if (onSelect || disabled) {
+    if (onSelect && disabled) {


again, check for disabled and return early

taion

There are still a few lint failures... broadly this code looks fine though

…react-bootstrap#2711

jackpetraitis · 2017-09-13T19:50:03Z

I think I fixed the linting errors. I ended up removing a Navbar test because it was a duplicate of another test.

I had issues writing tests for the NavItem + PagerItem code branches involving e.preventDefault. Essentially, once you add a disabled property on a tag you can no longer Simulate a click on it, and that would throw off all my sinon spy and stub logic. If you can figure out a way to do it I'd be all ears.

jackpetraitis · 2017-09-13T19:51:04Z

I also don't know why appveyor build is failing. It seems that build fix is outside of my control. The log mentioned something about trying to run an npm install command as administrator.

taion · 2017-09-13T19:51:42Z

AppVeyor is just flaky and janky.

@jquense What's up with the tsp.fn.log thing?

taion · 2017-10-19T23:11:32Z

ping @jquense

jquense · 2017-10-20T12:51:22Z

LGTM, the log this was just a convenience for logging why chaining assertions. It gets removed in the enzyme switch anyway

taion · 2017-10-20T20:13:20Z

I resolved the merge commit. Good to go on my end now, just need signoff

jackpetraitis changed the base branch from master to next September 12, 2017 17:10

joelseq mentioned this pull request Sep 12, 2017

fix(Navbar): Allow external links to fire #2789

Closed

jackpetraitiscdk added 3 commits September 12, 2017 23:32

preventDefault specified to work only for disabled && onSelect; it is…

2ceb474

… a fix for issue react-bootstrap#2711

removing prevent defaults from any onSelect logic for NavItem, Panel,…

ce5085e

… and PanelGroup

adding tests for Panel and NavItem changes. testing for whether preve…

cc7d536

…ntDefault is clicked and for whether the navbar collapses when it is clicked.

taion force-pushed the master branch from ff6ef3b to cc7d536 Compare September 13, 2017 03:36

taion mentioned this pull request Sep 13, 2017

fixes #2365 click on href with collapseOnSelect #2711

Closed

taion reviewed Sep 13, 2017

View reviewed changes

part of the fix for react-bootstrap#2711, removing preventDefault for…

bf8454d

… onSelect. also modifying code so it only will preventDefault if element is disabled

jquense reviewed Sep 13, 2017

View reviewed changes

refactoring the fix for issue react-bootstrap#2711

65200b4

taion approved these changes Sep 13, 2017

View reviewed changes

figured this test was unnecessary. also fixed some linting errors. for …

2bc169f

…react-bootstrap#2711

Merge branch 'next' into master

fc4bdde

zhanwenchen mentioned this pull request Oct 22, 2017

CollapseOnSelect in Responsive Navbars prevents external links from working #2365

Closed

taion merged commit 9140f14 into react-bootstrap:next Nov 8, 2017

renovate bot mentioned this pull request Mar 22, 2018

Update react-bootstrap to ^0.32.0 - autoclosed sbardian/DebtTrackerAPI#29

Closed

This was referenced Oct 10, 2018

Update dependency react-bootstrap to ^0.33.0 neolanders/redux-min-starter#4

Open

Update dependency react-bootstrap to ^0.33.0 - abandoned neolanders/min-starter-react-redux-axios#13

Open

This was referenced Jan 8, 2019

Update dependency react-bootstrap to v0.32.4 SonyaMoisset/Meme-Generator_REACT-REDUX#11

Merged

Update dependency react-bootstrap to v0.32.4 - autoclosed SonyaMoisset/RecipeFinder_REACT-REDUX#10

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

preventDefault specified to work only for disabled && onSelect; it is… #2790

preventDefault specified to work only for disabled && onSelect; it is… #2790

jackpetraitis commented Sep 12, 2017

taion commented Sep 12, 2017

jackpetraitis commented Sep 12, 2017

jquense commented Sep 12, 2017

taion commented Sep 12, 2017

jackpetraitis commented Sep 12, 2017

taion commented Sep 12, 2017

jackpetraitis commented Sep 12, 2017

jackpetraitis commented Sep 13, 2017

taion commented Sep 13, 2017

taion Sep 13, 2017

jquense Sep 13, 2017

jackpetraitis Sep 13, 2017

taion Sep 13, 2017

jackpetraitis Sep 13, 2017

jackpetraitis commented Sep 13, 2017 •

edited

Loading

jquense Sep 13, 2017

jackpetraitis Sep 13, 2017

jquense Sep 13, 2017

jackpetraitis Sep 13, 2017

taion left a comment

jackpetraitis commented Sep 13, 2017

jackpetraitis commented Sep 13, 2017

taion commented Sep 13, 2017

taion commented Oct 19, 2017

jquense commented Oct 20, 2017

taion commented Oct 20, 2017

preventDefault specified to work only for disabled && onSelect; it is… #2790

preventDefault specified to work only for disabled && onSelect; it is… #2790

Conversation

jackpetraitis commented Sep 12, 2017

taion commented Sep 12, 2017

jackpetraitis commented Sep 12, 2017

jquense commented Sep 12, 2017

taion commented Sep 12, 2017

jackpetraitis commented Sep 12, 2017

taion commented Sep 12, 2017

jackpetraitis commented Sep 12, 2017

jackpetraitis commented Sep 13, 2017

taion commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackpetraitis commented Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taion left a comment

Choose a reason for hiding this comment

jackpetraitis commented Sep 13, 2017

jackpetraitis commented Sep 13, 2017

taion commented Sep 13, 2017

taion commented Oct 19, 2017

jquense commented Oct 20, 2017

taion commented Oct 20, 2017

jackpetraitis commented Sep 13, 2017 •

edited

Loading