Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

Allow parent_className and parent_style attributes on dcc.Loading components #840

Merged
merged 11 commits into from
Aug 18, 2020

Conversation

wbrgss
Copy link
Contributor

@wbrgss wbrgss commented Aug 6, 2020

Fixes #831

I plan to make a test for this in the vein of the test app in #831 (comment)

@@ -53,7 +55,13 @@ export default class Loading extends Component {
const Spinner = isLoading && getSpinner(spinnerType);

return (
<div style={isLoading ? hiddenContainer : {}}>
<div
className={isLoading ? null : className}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the original behavior, modified in #740 which introduced the regression, the className was either applied to

  • the Spinner, if in a loading state
  • the this.props.children div parent, otherwise (at least, if this.props.children wasn't an Object or Function, for some reason. If it was, no className would be applied. See below for these three conditions.)

I'm attempting to mimic this behaviour here. Ideally this would be two different props, in the pattern of style and the new (in this PR) parent_style, but in order to fix the regression and have current apps working without breaking changes I think we need to replicate the old behaviour here.

This was the previous render return value:

if (loading_state && loading_state.is_loading) {
const Spinner = getSpinner(spinnerType);
return (
<div style={{visibility: 'hidden', position: 'relative'}}>
{this.props.children}
<Spinner
className={className}
style={{
visibility: 'visible',
position: 'absolute',
top: '0',
height: '100%',
width: '100%',
display: 'flex',
justifyContent: 'center',
alignItems: 'center',
...style,
}}
status={loading_state}
color={color}
debug={debug}
fullscreen={fullscreen}
/>
</div>
);
}
if (
type(this.props.children) !== 'Object' ||
type(this.props.children) !== 'Function'
) {
return <div className={className}>{this.props.children}</div>;
}
return this.props.children;

@wbrgss
Copy link
Contributor Author

wbrgss commented Aug 6, 2020

Some relevant comments from #740 that I wish to address in this PR:

One issue that stems from this is that the component itself would be moved "down" a level in the DOM, which might affect the CSS/layout of different apps.

(#740 (comment))

Some tests are currently failing because I didn't put className onto the wrapper div when loading is done, I only put it on the spinner itself, when the spinner is present. Is it useful for this class to be applied to the wrapper? At first glance I'd think this is actually unwanted, because then you can't use this class to target the spinner.

#740 (comment)

The problem here is that you do need to be able to apply a class to the wrapper in order to avoid the situation described in #831, and e.g. this community issue (and previously, this was the className prop of the Loading component). It's maybe not ideal to have this class possibly applied to two different elements, but this was the behavior before, and it might be OK considering it only applies to one element at a time depending on the loading state. cc @alexcjohnson

@wbrgss
Copy link
Contributor Author

wbrgss commented Aug 7, 2020

It's maybe not ideal to have this class possibly applied to two different elements, but this was the behavior before, and it might be OK considering it only applies to one element at a time depending on the loading state

An alternative to this would be to keep className for the Spinner, and add parent_className to accompany the parent_style in this PR. This wouldn't restore the previous behaviour before the regression, but rather provide a method for users relying on previous behaviour to fix their apps — namely by changing their dcc.Loading className prop to parent_className. The advantage is this would prevent the consistency problem with className conditionally assigned to different elements in the hierarchy depending on loading state.

What do you think @alexcjohnson? It's a breaking change, but arguably that ship has sailed already with DCC 1.9.1. If you think parent_className is preferable, I'll document parent_className in this PR and make sure to notify the users who have reported this issue so they can fix their apps.

@alexcjohnson
Copy link
Collaborator

Talking with @wbrgss just now, it seems like there was never really a use case for having the same className on both the spinner and the parent. There was a use case for the spinner className (to style the spinner itself) and a separate use case to style the parent (primarily to inherit sizing from its own parent). But if you happened to require both of these use cases at the same time, you'd be out of luck - one of them would break the other behavior.

Based on that, I think we can consider the previous behavior (prior to #740) to be a bug, and we can use @wbrgss's suggestion of adding parent_className as a separate prop as a clean way to support both use cases without them colliding.

@wbrgss
Copy link
Contributor Author

wbrgss commented Aug 12, 2020

Another question is where to put id — which is in the PropTypes, but doesn't (and hasn't ever, AFAIK) appear anywhere in the render function. I think it should be added.

I guess it would be consistent to add it as a prop of the Spinner but it still doesn't quite seem right to me, as when the app is first initialized, one could have a callback that uses an id, but the Spinner won't necessarily exist in the DOM at that time. But maybe that's OK? We could have parent_id — but I don't think that would fit with the Dash callback architecture, the stringification of id objects in pattern-matching callbacks, etc.

@alexcjohnson
Copy link
Collaborator

The only reason to want id in the DOM, AFAIK, is for CSS. For that purpose you could make a parent_id if you wanted to, but I'd argue className is better for CSS targeting anyway. The pattern-matching callbacks object-ID concept is useless, or at least extremely awkward, for CSS, so this would be string-only.

So sure, you can put id in the DOM, and for consistecy of naming I'd put it on the spinner next to style and className. But there's also no harm leaving it out of the DOM and having its sole purpose be to wire up callbacks.

@wbrgss
Copy link
Contributor Author

wbrgss commented Aug 14, 2020

But there's also no harm leaving it out of the DOM and having its sole purpose be to wire up callbacks.

OK, sounds good to me. Let's keep the scope of this to just address the original issue then, and leave out the id in the DOM.

@wbrgss wbrgss marked this pull request as ready for review August 17, 2020 14:29
@wbrgss wbrgss changed the title Allow className and parent_style attributes on dcc.Loading components Allow parent_className and parent_style attributes on dcc.Loading components Aug 17, 2020
@alexcjohnson
Copy link
Collaborator

@wbrgss this looks good to me. Just needs a changelog entry, and either update the failing test not to use output= or wait for me to address plotly/dash#1366

value=5,
),
])
app.layout = html.Div(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only manual changes here were removing the output=, input=, state= keyword arguments in the test_location_link test in order to work around plotly/dash#1366; the other changes (double " quotes, indentation) are formatting changes from blackening this file.

@wbrgss
Copy link
Contributor Author

wbrgss commented Aug 18, 2020

@alexcjohnson thanks for info re plotly/dash#1366; didn't realize that was the cause of the failing legacy tests. Tests are passing and CHANGELOG has been updated 🙂

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Excellent!

@wbrgss wbrgss merged commit 0ceb2de into dev Aug 18, 2020
@wbrgss wbrgss deleted the dcc-loading-className-regression branch August 18, 2020 19:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Graph dimension problem with dcc.Loading — regression (?) in DCC 1.9.1
2 participants