search

stratiformltd / react-loadable-visibility

A wrapper around react-loadable and @loadable/component to load elements once they are visible on the page.
BSD 3-Clause "New" or "Revised" License
1.01k stars 31 forks source link

Visibility container restricts LoadingComponent size/layout #3

Closed jahed closed 6 years ago

jahed commented 7 years ago

Since the LoadingComponent is wrapped in a div with an inline-block style, it can't span the entire width of the grandparent (parent being the div). This is solvable by letting the user set the container style themselves for their needs. Sometimes leaving the div's default block is good enough.

You can see this change here:

https://github.com/stratiformltd/react-loadable-visibility/compare/master...jahed:feature/containerStyle?expand=1

I didn't send a pull request as it is a breaking change and possibly unwanted. I didn't default containerStyle to inline-block for backwards compatibility as it seemed like an odd default for my usages.

Of course, this doesn't solve the problem where say you have a grandparent which using flex which prevents the LoadingComponent from working off it (since it's not a direct descendant, unlike LoadableComponent). Since we need a container ref to check visibility, the only other option I can see is to ask the user to pass the ref prop down through their LoadingComponent all the way to wherever the DOM components are rendered.

Just thought I'd bring all of this up, to see if it's considered an issue.

tazsingh commented 7 years ago

Thanks for bringing this up @jahed. Definitely something that I thought of when building the library but was unsure if it would be an issue for anyone.

Now that you've highlighted that it is, I think moving to accepting a className would be an appropriate path forward. If the className prop is empty, then can default to the existing style={{display: 'inline-block'}}.

What do you think about that?

I likely won't have a chance to physically do this for a little while as I'm on vacation. But if you want to throw together a PR for this, I'd be happy to review and merge. Will cut a release with it when I'm back from vacation in a few weeks.

jahed commented 7 years ago

I think it's strange for a component to decide if it adds style props based on if a className is provided. className might be used for other things like selectors in integration tests where you wouldn't expect it to effect styling.

If we really do need to keep the inline-block default, I'd suggest then just provide className functionality so that users that don't want inline-block can override it with an !important and add any other styles they need (e.g. flex) via className if needed. It's not elegant, but I think it's fine for the sake of backwards-compatibility without side-effects. Allowing className is a pretty common requirement in general anyway.

The only downside I can see with this approach is that we're assuming users are using CSS in their projects (in order to style via className) opposed to inline styles. Some users prefer styling with className (CSS Modules), others like using style. I personally don't mind either.

We can defer this discussion later when you're back from vacation. (I'm also on vacation, in a sense 😉)

tazsingh commented 6 years ago

@jahed I'm back in town for today but then will be offline for a few days again, and will then be back online for a while.

I like your suggestion of keeping the style but then overriding via a className. It's more predictable as you mentioned. Let's do that 👍

I'll whip up that change.