fix(Accordion): Use empty SelectableContext to prevent dropdown from closing accordion

[kyletsang]

Fixes #4176

I moved the SelectableContext consumption to NavDropdown because it looks like this context is used in conjunction with Nav in triggering onSelect for NavDropdowns.

I've added the test in Nav to make sure I didn't break anything in that API and another test in Accordion to make sure the a dropdown would not trigger the accordion context.

This would also close the PR #4772.

[taion]

cc @jquense does this API change make sense to you?

[taion]

i wonder if we want to make as customizable in this context, then.

with this change, rendering a <Dropdown> with whatever styling inside a <Nav> now won't do the same thing as rendering a <NavDropdown>

which is probably okay, but maybe a bit more customizability here would help

[kyletsang]

@taion, I just revisited this and I realized anything that implements AbstractNav would suffer the same issue of collapsing the accordion inadvertently. I'm thinking a better solution would be to wrap everything in AccordionCollapse with <SelectableContext.Provider value={null}>?

ie. in AccordionCollapse.js

return (
  <SelectableContext.Provider value={null}>
    <Collapse ref={ref} in={contextEventKey === eventKey} {...props}>
      <div>{React.Children.only(children)}</div>
    </Collapse>
  </SelectableContext.Provider>
);

What do you think?

[taion]

That could work. We might want to do that in general wherever we consume this context, then?

Could also use a different context here, but I guess that'd be breaking.

I'm not sure. Is it definitely the case that AbstractNav shouldn't call into the parent onSelect from an accordion parent?

[kyletsang]

TBH, I can't think of any case where elements within the collapse would want to trigger the accordion parent.

[taion]

well, it's sort of like nested navigation... idk

we can try that out, though

[kyletsang]

I'll update the PR anyway since the original solution doesn't cover all cases. Others can chime in and we can close this if it's no good.

@jquense @bpas247 @mxschmitt

[eshwar-quartic]

@taion Any ETA on getting this merged?

[jquense]

I'm a bit concerned this change will break anyone using a custom dropdown in a navbar. I think maybe the underlying issue is we are reusing SelectableContext when we should probably have a different SelectableContext "instances" for each, lack of a better word, group of selectable behaviors. Then the contexts would talk past each other but maintain a unified public interface.

something like:

NavSelectableContext = createSelectableContext();
AccordianSelectableContext = createSelectableContext()

[taion]

Oh, that sounds reasonable to me. Can we make that change for v1, though, or does it need to be deferred for v2?

[jquense]

I think this can go into a v1, it is maintaining the same API, and fixes a bug...is the SelectionContext public?

[taion]

It's not explicitly documented nor exposed at top-level. I think we may have pointed people toward it for cases where they need more fine-grained control, though.

[kyletsang]

Oops I forgot to update the title here - it no longer reflects the new change.

What I ended up doing is wrapping the AccordionCollapse with a null SelectableContext. This prevents the elements inside the collapse from grabbing the context.

Looking at the code I believe the only element that uses SelectableContext is the toggle. I don't think the contents of the collapse need the root SelectableContext from Accordion.... does it?

@jquense

[tmm1]

Thanks for this fix! Any chance of a 1.3.1 release soon with this included?

[tmm1]

@taion friendly ping for a new release sometime :)

[kyletsang]

@tmm1 this is available in 1.4.0

[tmm1]

Thanks! I missed that on https://github.com/react-bootstrap/react-bootstrap/releases