Dashboard Plugin: remove jumpiness when uppy loads

[lakesare]

No description provided.

[arturi]

This looks good from the first test, thanks! 🎉 ✨

I’d probably do it via a classname from the top container, similar to how we have uppy-Dashboard--isAddFilesPanelVisible. Then we can, for instance, animate the visibility later, if we want to. Plus for consistency.

And isDashboardVisible can become a prop in index.js vs the Dashboard.js component, what do you think?

[lakesare]

@arturi,

I see what you mean about the prop and a class! The reason I didn't use a prop, and didn't use a class, is because I didn't want to disperse the logic across multiple files (logic is described in a comment, is it clear btw? I made it shorter and more unclear I think).

If we were using a prop, we'd need to mention in index.js why exactly our dashboard insides shouldn't be visible when props.containerWidth isn't yet set.
Then we would also need to mention in Dashboard.js why exactly we can't apply { display: none; } to dashboardClassName (and can apply { visibility: hidden } to dashboardClassName, but shouldn't), leaving .uppy-Dashboard-innerWrap the only candidate for applying invisibility to.

Do you think using props and className is a good exchange for dispersing some logic in this case?

[arturi]

I see your points too, thanks! And I agree with them, partly. But since all the other logic is in index.js, and other things like animation, isClosing and etc are done via classes (or I was thinking we could switch do data-attributes for logic instead of class names, which would be kept for styles only), the isDashboardVisible helper is lonely there. I think I’d prefer a prop isDashboardVisible, and then the visibility set via CSS (with a comment in both CSS and in index.js): .uppy--isDashboardVisible .uppy-Dashboard-innerWrap. I think it might also be better to keep the logic closer to ResizeObserver code?

It could also be nice to add a setTimeout for a second or so, which would set that props.isDashboardVisible to true regardless of whether props.containerWidth is 0 or not. To safeguard for some browsers where the ResizeObserver fails and never actually calculates anything, then at least users get mobile version (which is the case before this PR). Otherwise, in case of a failure, it will always stay in visibility: hidden mode. What do you think?

[lakesare]

Good point about keeping it close to ResizeObserver, and if we add setTimeout (good idea, I'm not confident in that polyfill) we'd have no choice but to make it a prop.
I'll send another PR later today.

[arturi]

Thank you! Updating this one works, too! :)

We can have a call about it if needed, let me know on Slack if so.

[lakesare]

@arturi, pushed a new version!
Please pay some attention to the new this.resizeObserver.disconnect() instead of previous this.resizeObserver.unobserve(els) solution, was there some reason we were using latter rather than the former?

[arturi]

So disconnect just unobserves everything, no need to specify, right? I guess there’s no reason not to use it then :) Any particular reason for switching from visibility to opacity? Either should work, right?

[lakesare]

So disconnect just unobserves everything, no need to specify, right? I guess there’s no reason not to use it then :)

Yes, exactly. Sounds good!

Any particular reason for switching from visibility to opacity? Either should work, right?

Both work, but you mentioned possible animation in the future, and I thought we may want to use opacity while we're at it right away, since it's more easily animatable.

[arturi]

Agreed and merged! Thank you! :)