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

Provide a way to disable rendering `div` for `Router` #63

Closed
chenxsan opened this Issue Jun 9, 2018 · 22 comments

Comments

Projects
None yet
@chenxsan
Copy link

chenxsan commented Jun 9, 2018

Feature Request?

My root node has css like this:

display: flex;
flex-direction: column;
height: 100vh;

When Router renders a div, it breaks my layout:

image

I found there's no way to disable it in the code https://github.com/reach/router/blob/master/src/index.js#L336.

It would be great if I can pass false or null value to component prop of Router like this to disable rendering the div:

<Router component={null}>
 ...
</Router>
@joao-alberto

This comment has been minimized.

Copy link

joao-alberto commented Jun 9, 2018

My proposal is use Fragment when is available and let dev set the style of div when Fragment is not available.

Im most of cases the style of wrapper will be:

height: 100%;
width: 100%;

What your think @chenxsan?

@ryanflorence

This comment has been minimized.

Copy link
Contributor

ryanflorence commented Jun 9, 2018

Couple of things:

  • The div is necessary to manage focus, so we won't be providing a way to disable that
  • Router is the div, so any props you pass to it go to your div

Therefore:

<Router id="__OkMarvin__">
 {/* ... */}
</Router>
@chenxsan

This comment has been minimized.

Copy link

chenxsan commented Jun 10, 2018

@ryanflorence #__OkMarvin__ is set on React root node, there's no way to pass it. Also I'm using reach on server side rendering only, no client side rehydrate, so no foucs is needed in my case.

@chenxsan

This comment has been minimized.

Copy link

chenxsan commented Jun 10, 2018

@joao-alberto Yep, I've tried <Router component={React.Fragment} />, but React.Fragment won't accept style prop causing lots of warning in console.

image

@ryanflorence

This comment has been minimized.

Copy link
Contributor

ryanflorence commented Jun 12, 2018

You'll need to move that ID from the root node to the router, or change your CSS some other way, but we won't be making the div optional because accessibility is a primary concern for this package.

@daviestar

This comment has been minimized.

Copy link

daviestar commented Jun 19, 2018

Could this be reconsidered?

I am working on a site where it makes a lot of sense to stick a <Router> component inside an SVG, where it breaks with the containing div.

@daviestar

This comment has been minimized.

Copy link

daviestar commented Jun 22, 2018

How about when the router isn't considered primary:

<Router basepath="/timeline" primary={false}>
  // ...
</Router>

which doesn't take focus anyway?

@zxuqian

This comment has been minimized.

Copy link

zxuqian commented Jun 30, 2018

I demand this requirement too. One of my layout component has three sections which are top level. But when it becomes the route component, the layout becomes a mess.

@adamyonk

This comment has been minimized.

Copy link

adamyonk commented Jul 13, 2018

It would be nice to have some sort of escape hatch, like being able to pass in your own component to be used <Router as={OtherComponent}>.

@dnutels

This comment has been minimized.

Copy link

dnutels commented Jul 25, 2018

@adamyonk

Works for me:

import Root from './root/Root';

<Router component={Root}>
    <Dashboard path="/" default />
</Router>

@daviestar Can't you change it to like g or some other SVG element?

EDIT: in the component case, any Back/Forward actions fail with:

Uncaught TypeError: this.node.focus is not a function...

Alas...

@sshmyg

This comment has been minimized.

Copy link

sshmyg commented Jul 31, 2018

@ryanflorence #63 (comment)
Maybe user can make a decision about accessibility? Your additional div broke all design, and if I use css-in-js it become a big problem and not helpful feature.

I think wrapper by default it's ok, but please, provide some solution how to use Router without additional wrapper. Maybe some diff component with appropriate props, or something.

Outline is largely an accessibility feature and should be available on elements to indicate focus during keyboard navigation.

So outline: none; it's a bug in Router accessibility!

Thank you.

@sshmyg

This comment has been minimized.

Copy link

sshmyg commented Jul 31, 2018

And it's very strange, first wrapper I can hide with <Router component={RouterComponent}>, but all nested routes wraps with default div. And if primary is false, than we have no any tabindex etc, just unneeded wrapper.

@ryanflorence Maybe behaviour should be the same for all routes? Top and nested.

@romanlex

This comment has been minimized.

Copy link

romanlex commented Aug 1, 2018

if primary is false I think needed use Fragment, not div

@adamscybot

This comment has been minimized.

Copy link

adamscybot commented Aug 28, 2018

I've carried the styling conversation over to #145, which is a more scoped issue than this one (which would potentially remove the accessibility aspect of this library)

@aruprakshit

This comment has been minimized.

Copy link

aruprakshit commented Sep 7, 2018

@romanlex Could you please explain what did you mean by the primary={false} trick?

@sshmyg

This comment has been minimized.

Copy link

sshmyg commented Sep 7, 2018

@aruprakshit primary={false} remove div wrapper. But only for firs route. All others nested routes, has this wrapper and not depends on primary prop;

@aruprakshit

This comment has been minimized.

Copy link

aruprakshit commented Sep 7, 2018

@sshmyg I can see that the div is not removed, only some props it has before is removed. check this example.

@sshmyg

This comment has been minimized.

Copy link

sshmyg commented Sep 7, 2018

@aruprakshit Probably @romanlex want to add such ability. Disable accessibility in such way.

@janosh

This comment has been minimized.

Copy link

janosh commented Sep 28, 2018

What is the issue with reusing the div into which React is mounted as proposed by @KyleAMathews here.

@ironblock

This comment has been minimized.

Copy link

ironblock commented Oct 9, 2018

I have the same issue with the wrapping <div> breaking flexbox layouts. I'll offer my own fragile, hacky workaround in case it gets one of y'all unstuck too:

The wrapper <div>s created by <FocusHandlerImpl> have enough in common by default to allow a CSS selector to target them. Specifically, they all set role, tabindex, and are <div>s. Reading the source, it looks like role will always default to "group", so if you're feeling saucy you can do something like

div[role="group"][tabindex] {
  flex: 1;
  // Other rules that don't break a11y 😊
}

It's not ideal, but this got my layout un-buggered, and I'm only planning on applying flex: 1 (and maybe a light display: flex as needed). It should also be easy to override the specificity of this selector if it ends up matching other things in your app. Long term, I like the proposal in #145 to set/share common props on these elements.

@lkleuver

This comment has been minimized.

Copy link

lkleuver commented Dec 6, 2018

I understand that removing the wrapper div goes against the goal of reach (Router)
@ryanflorence A solution for me would be to ignore children without a path prop.
(I'm using a grid layout and want children to fill in slots of the grid)

@j3l11234

This comment has been minimized.

Copy link

j3l11234 commented Jan 15, 2019

Couple of things:

  • The div is necessary to manage focus, so we won't be providing a way to disable that
  • Router is the div, so any props you pass to it go to your div

Therefore:

<Router id="__OkMarvin__">
 {/* ... */}
</Router>

@ryanflorence
how about the nested Route? the props did not pass to children router. I just want to pass the classname to the wrapper element.

https://github.com/AuthurWang/router/blob/master/src/index.js#L206

      let clone = React.cloneElement(
        element,
        props,
        element.props.children ? (
          <Router primary={primary}>{element.props.children}</Router>
        ) : (
          undefined
        )
      );
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment