Linear progress bar. #88

michalchudziak · 2016-11-26T16:07:27Z

Implementation of linear progress bar in two states.

Determinate:
When indicators are determinate they indicate how long an operation will take when the percentage complete is detectable.

Indeterminate:
When indicators are indeterminate they request that the user wait while something finishes when it’s not necessary to indicate how long it will take.

satya164 · 2016-11-26T20:54:40Z

src/index.js

@@ -12,6 +12,8 @@ export { default as Paper } from './components/Paper';
 export { default as RadioButton } from './components/RadioButton';
 export { default as TouchableRipple } from './components/TouchableRipple';

+export * as Progress from './components/Progress';


This file should not know how individual components are exported. Let's change it to

export { default as Progress } from './components/Progress

satya164 · 2016-11-26T20:55:55Z

src/components/Progress/index.js

@@ -0,0 +1 @@
+export { default as Linear } from './Linear';


Can be changed to

import Linear from './Linear'; export default { Linear, }

Any benefits? Not sure if thats the same though. Chudziak's code has named exports whereas yours exports a default object with properties. Not sure if that's guarnateed to behave the same way or it's just Babel inter-op with require, but yeah, I'd stick with the former.

I think then we can use export { default as Progress } from './components/Progress from the comment above

@mike866 No I wanted to export an object here instead of named exports.

satya164 · 2016-11-26T20:56:46Z

src/components/Progress/Linear.js

@@ -0,0 +1,193 @@
+/* @flow */


As a convention I usually name the component (and file) as ProgressLinear instead of Linear. That way when it's shown in error messages ProgressLinear will be shown which is more informative.

satya164 · 2016-11-26T21:05:46Z

src/components/Progress/Linear.js

+
+type Props = {
+  color: string;
+  emptyColor: string;


This is same as style={{ backgroundColor: color }}, right? In that case, let's remove the prop

satya164 · 2016-11-26T21:06:16Z

src/components/Progress/Linear.js

+  };
+
+  static defaultProps: Props = {
+    color: blueA200,


It should use the accent color from theme by default.

satya164 · 2016-11-26T21:06:47Z

src/components/Progress/Linear.js

+
+  static defaultProps: Props = {
+    color: blueA200,
+    emptyColor: blue100,


This could be achieved by lightening the color I believe.

grabbou · 2016-11-28T13:18:54Z

src/components/Progress/Linear.js

+
+import { blueA200, blue100 } from '../../styles/colors';
+
+const roundProgress = (progress: ?number): number => Math.min(Math.max((progress || 0), 0), 1);


I'd add description, took me a while to figure out. Basically it's used to return a number between bounds (0, 1), otherwise upper or lower limit.

satya164 · 2016-11-28T13:30:46Z

example/src/ProgressExample.js

+  constructor(props) {
+    super(props);
+
+    const interval = setInterval(() => {


I think better to move this out of the state. This is something the UI doesn't depend upon, so shouldn't be in state. I'd do this._interval

satya164 · 2016-11-29T01:55:12Z

src/components/Progress/ProgressLinear.js

+      this._startIndeterminateAnimation();
+    }
+
+    Animated.spring(this.state.visibleAnimationValue, {


This needs to check if this.props.visible is true

satya164 · 2016-11-29T01:57:19Z

src/components/Progress/ProgressLinear.js

+
+    const defaultBarXPosition = indeterminateWidth / (1 + indeterminateWidth);
+
+    const { width, height } = StyleSheet.flatten(props.style);


We shouldn't rely on user to pass height and width. We should have default height in styles and width should be detected automatically using onLayout

Also instead of animating height, we should use a scale animation (will need to use together with translate since we don't have transform origin support)

satya164 · 2016-11-29T01:59:17Z

src/components/Progress/ProgressLinear.js

+    const { width, height } = StyleSheet.flatten(props.style);
+
+    this.state = {
+      progressAnimationValue: new Animated.Value(props.indeterminate ? indeterminateWidth : progress),


Can we use scale instead of animating width? Scale animations are much smoother than width/height.

satya164 · 2016-11-29T02:04:19Z

example/src/ProgressExample.js

+  constructor(props) {
+    super(props);
+
+    this.interval = setInterval(() => {


Can we move this to componentDidMount instead? Also change it to this._interval.

Doesn't matter here, but it's better to have side effects in componentDidMount, since in plain React you don't usually want them to trigger on the server while server-rendering.

satya164 · 2016-11-29T02:06:39Z

example/src/ProgressExample.js

+    };
+  }
+
+  state = {


This is declared twice. I think better to get rid of the constructor. And move interval stuff to componentDidMount.

satya164 · 2016-11-29T02:08:09Z

example/src/ProgressExample.js

+  render() {
+    return (
+      <View style={styles.container}>
+        <Headline style={styles.text}>Linear</Headline>


Headline, Subheading etc have a default vertical margin of 4px. I think you don't need the additional style

satya164 · 2016-11-29T02:08:48Z

src/components/Progress/ProgressLinear.js

+  Easing,
+  View,
+} from 'react-native';
+import color from 'color';


nit: remove new line between imports

satya164 · 2016-11-29T02:09:49Z

src/components/Progress/ProgressLinear.js

+  constructor(props: Props) {
+    super(props);
+
+    const progress: number = roundProgress(props.progress);


nit: remove new lines between const declarations

satya164 · 2016-11-29T02:10:28Z

src/components/Progress/ProgressLinear.js

+      easing: Easing.linear,
+      isInteraction: false,
+    }).start((animationState) => {
+      if (animationState.finished) {


I wish we had Animated.loop

satya164 · 2016-11-29T02:11:15Z

src/components/Progress/ProgressLinear.js

+    } = this.props;
+    const { width, height } = this.state;
+
+    const progressColor = this.props.color || this.props.theme.colors.accent;


Destructure theme above

…nto progress

ferrannp · 2017-09-19T13:33:23Z

This is stale. Re-check issue: #11.

michalchudziak added 2 commits November 26, 2016 17:04

Linear progress bar.

66c8a95

Remove unused FlowFixMe.

0e5b9f3

satya164 reviewed Nov 26, 2016

View reviewed changes

grabbou reviewed Nov 28, 2016

View reviewed changes

Rebase.

40dc0bd

satya164 requested changes Nov 28, 2016

View reviewed changes

michalchudziak added 3 commits November 28, 2016 20:29

Visible prop for progress bar + code refactor.

532827f

Move interval.

0d3560d

remove unused interval from state.

7d81c22

satya164 requested changes Nov 29, 2016

View reviewed changes

michalchudziak and others added 7 commits November 29, 2016 11:05

Change height animation to scale + code refactor.

3e11d0d

Unused style removal.

a524848

Unused style removal.

c422b60

Merge branch 'master' into progress

ad52c46

fix: use translate animation for progress

a609500

Remove unused var

71f8ec5

Merge branch 'master' of github.com:ahmedlhanafy/react-native-paper i…

28fa4fd

…nto progress

ahmedlhanafy assigned satya164 May 10, 2017

ferrannp closed this Sep 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linear progress bar. #88

Linear progress bar. #88

michalchudziak commented Nov 26, 2016

satya164 Nov 26, 2016

satya164 Nov 26, 2016

grabbou Nov 28, 2016 •

edited

Loading

michalchudziak Nov 28, 2016

satya164 Nov 28, 2016

satya164 Nov 26, 2016

satya164 Nov 26, 2016

satya164 Nov 26, 2016

satya164 Nov 26, 2016

grabbou Nov 28, 2016

satya164 Nov 28, 2016

satya164 Nov 29, 2016

satya164 Nov 29, 2016

satya164 Nov 29, 2016

satya164 Nov 29, 2016

satya164 Nov 29, 2016

satya164 Nov 29, 2016

satya164 Nov 29, 2016

satya164 Nov 29, 2016

satya164 Nov 29, 2016

satya164 Nov 29, 2016

ferrannp commented Sep 19, 2017


		import { blueA200, blue100 } from '../../styles/colors';

		const roundProgress = (progress: ?number): number => Math.min(Math.max((progress \|\| 0), 0), 1);


		const defaultBarXPosition = indeterminateWidth / (1 + indeterminateWidth);

		const { width, height } = StyleSheet.flatten(props.style);

Linear progress bar. #88

Linear progress bar. #88

Conversation

michalchudziak commented Nov 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grabbou Nov 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferrannp commented Sep 19, 2017

grabbou Nov 28, 2016 •

edited

Loading