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

Loading component #352

Merged
merged 66 commits into from
Feb 28, 2019
Merged

Loading component #352

merged 66 commits into from
Feb 28, 2019

Conversation

valentijnnieman
Copy link
Contributor

@valentijnnieman valentijnnieman commented Oct 30, 2018

This is the new Loading component for the new Loading States api issue 267. For now it has a basic default spinner.

Please note!
I've had to make a small change to extract-meta to not pick up .jsx files, so that we can have .jsx files in src/ that are not supposed to be mapped to Dash components. This is so that I could split up all the spinners into separate files, that we don't want Dash to pick up as components - instead they should be JS only components that get used under the hood. There's probably a better fix here, let me know your thoughts please.

TODO:

  • Add more default spinners, and a prop for selecting which one to use (loading_screen_style = 'basic' | 'overlay' | 'spinner' | 'empty-div' | or something similar)
  • Support for full-screen spinners
  • Add props for styling the default spinner's colours and other
  • Add support for handling which prop caused loading time (once in renderer PR)
  • Allow for React components that don't get picked up by extract-meta, so we can have a file structure that allows separating of concerns.
  • Look into adding support for custom loading components, with this PR perhaps

SPINNERS:

edit: I'm also showing the component and prop Name in the HTML, just to show off that that information is being passed. In the final version of this component, I'll remove it (or at least change how it looks)

edit2: I've fixed the Demo app and I've added a Loading Demo app that you can fire up to check out all the spinners. npm start!

@T4rk1n
Copy link
Contributor

T4rk1n commented Oct 30, 2018

@valentijnnieman You wrap the component you want to have a loading with this loading component ?

@valentijnnieman
Copy link
Contributor Author

@bpostlethwaite I think you mentioned specific loading spinners for Plotly on Friday? If there are any that you think we should incorporate into this component, could you link them here? Thanks in advance :)

@bpostlethwaite
Copy link
Member

https://github.com/plotly/streambed/blob/master/shelly/webapp/static/webapp/images/plotly-logo-loop.gif

But this one doesn't actually seem to have looping enabled. You'll need to open a GIF editor and enable looping.

I am pretty sure we should edit this one in streambed to support looping as well...

@bpostlethwaite
Copy link
Member

If you create a loop version let me know and I'll PR that back into streambed

@valentijnnieman
Copy link
Contributor Author

Hmm it's very small and blurry. I don't think we should have that as a default loading spinner, would rather have something in like we have here now! Taken from http://tobiasahlin.com/spinkit/ .

@valentijnnieman
Copy link
Contributor Author

I made this https://codepen.io/Chunkydory/pen/qQOrwB which is a little bit campy! But I kind of like it. @cldougl also pointed out that this exists! https://codepen.io/doeg/pen/RWGoLR

@valentijnnieman
Copy link
Contributor Author

Here's the pen of the cube spinner I just added: https://codepen.io/Chunkydory/pen/aQdpPL

@Marc-Andre-Rivet
Copy link
Contributor

@valentijnnieman No specific suggestion, just a curated list of spinners
https://github.com/yangshun/awesome-spinners

@valentijnnieman valentijnnieman changed the title [WIP]Loading component Loading component Nov 13, 2018
@@ -33,6 +34,7 @@
"babel-preset-env": "^1.7.0",
"babel-preset-react": "^6.24.1",
"builder": "3.2.2",
"color": "^3.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New dependency used for darkening hex colors, used in the CubeSpinner component. It allows users to provide a color, and the component will figure out the appropriate darker colors that are used in the cube.

);
};

export default LoadingDemo;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New Demo app that shows off the spinners.

@@ -39,7 +39,7 @@ function parseFile(filepath) {
const urlpath = filepath.split(path.sep).join('/');
let src;

if (!['.jsx', '.js'].includes(path.extname(filepath))) {
if (!['.js'].includes(path.extname(filepath))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We currently do not have any .jsx files in our source code, so I figured they could be used in a way to denote components that should not be picked up by extract-meta, so that we can use them internally in JS. It's an easy solution to have the spinners, in this case, in separate files and not in one huge Loading.react.js file.

<link rel="stylesheet" href="//cdnjs.cloudflare.com/ajax/libs/codemirror/5.0.0/theme/xq-light.min.css" />
<link rel="stylesheet" href="https://unpkg.com/react-select@1.0.0-rc.3/dist/react-select.css" />
<link rel="stylesheet" href="https://cdn.rawgit.com/chriddyp/abcbc02565dd495b676c3269240e09ca/raw/816de7d5c5d5626e3f3cac8e967070aa15da77e2/rc-slider.css"
/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are no longer needed since we supply these CSS files ourselves in the source code (using style-loader etc)

<script type="text/javascript" src="//cdnjs.cloudflare.com/ajax/libs/codemirror/5.0.0/codemirror.min.js"></script>
<script type="text/javascript" src="//cdnjs.cloudflare.com/ajax/libs/codemirror/5.0.0/mode/javascript/javascript.min.js"></script>
<script type="text/javascript" src="https://unpkg.com/react@15/dist/react.js"></script>
<script type="text/javascript" src="https://unpkg.com/react-dom@15/dist/react-dom.js"></script>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're supplying React in the devDependencies now, so this is no longer needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

So I dug through this because it was not obvious to me how the dependencies had changed based on the webpack.config.js and webpack.server.config.js changes in this PR -- they would not affect these script dependencies.

webpack.serve.config.js sets config.externals = undefined which makes webpack.config.js evaluate 'externals' in overrides to true which does not set react, react-dom, etc. as defaults.

Not going to ask for changes as this is out of scope, just documenting my own confusion. I think this could be improved on later.

@@ -55,7 +55,7 @@ module.exports = (env, argv) => {
module: {
rules: [
{
test: /\.js$/,
test: /\.jsx?$/,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

.jsx files weren't being picked up in the Webpack configs, this will add both .js and .jsx files.

config.output = {
filename: './output.js',
path: path.resolve(__dirname),
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes the Demo app.

PropTypes.node,
]),

type: PropTypes.oneOf(['graph', 'cube', 'circle', 'dot', 'default']),
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to add a description docstring here.

<h3 className="dash-loading-title">
Loading {status.component_name}
's {status.prop_name}
</h3>
Copy link
Contributor

Choose a reason for hiding this comment

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

😸

@valentijnnieman
Copy link
Contributor Author

Released as prerelease 0.44.0rc2

CHANGELOG.md Outdated
@@ -2,6 +2,9 @@
All notable changes to this project will be documented in this file.
This project adheres to [Semantic Versioning](http://semver.org/).

## [0.44.0] - 2019-02-13
### Added
- Loading component
Copy link
Contributor

Choose a reason for hiding this comment

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

Unreleased, revert version change for final merge. Add issue link.

Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

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

Mostly minor comments and questions

dash_core_components/CircleSpinner.py Outdated Show resolved Hide resolved
src/components/DatePickerRange.react.js Show resolved Hide resolved
src/components/Loading/Loading.react.js Outdated Show resolved Hide resolved
src/components/Loading/Loading.react.js Outdated Show resolved Hide resolved
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

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

Did not mean to approve just yet :)

@Marc-Andre-Rivet Marc-Andre-Rivet dismissed stale reviews from chriddyp and themself February 13, 2019 18:27

Mistake. Not quite there yet!

@valentijnnieman
Copy link
Contributor Author

(Re) released as rc 0.43.0rc3 including fixes

R.type(this.props.children) !== 'Object' ||
R.type(this.props.children) !== 'Function'
) {
return <div className={className}>{this.props.children}</div>;
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet Feb 15, 2019

Choose a reason for hiding this comment

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

We're not using the id passed through props for id+key of this component, normal and loading cases. Might cause similar problems as what we've seen with other comps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we're setting key anywhere in the components, so I don't think it'll cause problems. The key is set in dash-renderer, however. But, you're right - I should add the id prop here!

@eromoe
Copy link

eromoe commented Feb 26, 2019

I have installed 0.44.0rc2 , but no loading shows,

Is something wrong with my installed packages ? :

dash==0.36.0  # The core dash backend
dash-html-components==0.13.5  # HTML components
dash-core-components==0.44.0rc2  # Supercharged components
dash-table==3.1.11  # Interactive DataTable component (new!)
dash-daq==0.1.0  # DAQ components (newly open-sourced!)
dash-bootstrap-components

@valentijnnieman
Copy link
Contributor Author

Hi @eromoe - the packages you'd need are:

dash_html_components==0.14.0rc21
dash_renderer==0.18.0rc4
dash_core_components==0.43.0rc3

It looks like you're missing dash_renderer==0.18.0rc4!

Here's a link to the forums with a more detailed example of how to use this.

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.

8 participants