Feature/feature timer component #9

Bhuvneshwar-MB · 2020-04-30T12:13:48Z

I Have Implemented a Time r Component under components on Timer Screen

MDHayes

Some feedback:

The name TimerComponent seems a little redundant in the components directory. Maybe change this to TimerButton?
Leads onto, this needs a TouchableOpacity wrapping it to start/stop the timer and state that monitors it being started. I think we can do the within the component, although it would give some added flexibility if we passed them in as props. What do you think?
We're using styles here differently than other places ie Vinod is using const { textContainerStyle, textStyle } = styles rather than styles.stylename
Remove console.log
Magic numbers in the timer, the code I suggested had the default set in config this is good practice and will help in future if we want to increase the timer.
I notice the colours are specific to where they are implemented... what if we changed our minds and want the green to be #52D9B3?

…rked on suggestions

Bhuvneshwar-MB · 2020-05-01T06:02:03Z

The name TimerComponent seems a little redundant in the components directory. Maybe change this to TimerButton? - Done
Leads onto, this needs a TouchableOpacity wrapping it to start/stop the timer and state that monitors it being started. I think we can do the within the component, although it would give some added flexibility if we passed them in as props. What do you think? - i think here is not any start.stop in UI so that i did that.
We're using styles here differently than other places ie Vinod is using const { textContainerStyle, textStyle } = styles rather than styles.stylename - done
Remove console.log - done
Magic numbers in the timer, the code I suggested had the default set in config this is good practice and will help in future if we want to increase the timer. - done
I notice the colours are specific to where they are implemented... what if we changed our minds and want the green to be #52D9B3? - if we want to change green color, definitely its changes to everywhere we used green color or if we want to add new color then we can do that. What you suggest?

Bhuvneshwar-MB · 2020-05-01T07:46:49Z

PR refreshed as now use only FontAwesome for icons.

MDHayes

This needs some more work. Comments inline with the code. Specifically I think this should be reusable on the home page.

You also asked about colors. I've been recommending we have colors generic rather than implementation specific. I think its always easier to add special cases than to have many colors.

I guess this comes down to preference but something to think about when doing colors in future.

MDHayes · 2020-05-01T07:46:43Z

app/components/TimerButton.js

+  const renderTimer = () => {
+    return (
+      <View>
+        {timer === 0 ? (
+          <FontAwesome5 name={'thumbs-up'} size={30} color={white} />
+        ) : (
+          <Text style={textSeconds}>{timer}</Text>
+        )}
+      </View>
+    )
+  }


Why are we doing a function like this? It should be in the main return or split out. I think this is small enough to put in the return?

Also this button should be reused on the home screen:

That is why you need the TouchableOpcity on it. You would also need to make the icon here generic (was can pass in a name) and the text value changes.

At homeScreen we just need to show image. there are no progress needed and no state requires for the same. so that we use image at home screen and Circular progress on Timer screen.

I disagree. The home screen has an icon and text the same, same styling/spacing. I think we can make this one component.

Ok i have use same component for both Home Screen and Timer Screen by using props.

MDHayes · 2020-05-01T07:48:15Z

app/components/TimerButton.js

+    }
+  }, [timer])
+
+  const fill = 100 - timer * 5


Magic numbers here. What would happen to the animation if we changed the defaultTimer time to 40 seconds?

Okay then. What should i do?

Can you have a think about a solution first?

what you think about this :

const fill = 100 - (timer * 100) / timerDefault

yeah that looks good.

MDHayes · 2020-05-01T10:15:50Z

@Bhuvneshwar-MB let me know when this is good to review. Also merge with develop so there's no conflicts in the PR.

Bhuvneshwar-MB · 2020-05-01T10:23:52Z

I am working on conflicts. i thing some other branch merged in develop branch

Bhuvneshwar-MB · 2020-05-01T11:10:04Z

Can you please review now?

Feature/settings screen actions

MDHayes

We can do a better job on making the TimerButton more reusable and not having functions returning JSX. Check with Vinod and see if you can work together to improve it, he improved the SelectCell a lot in a similar way.

MDHayes · 2020-05-01T11:51:05Z

app/components/TimerButton.js

+  const renderCircularProgress = () => {
+    return (
+      <AnimatedCircularProgress
+        style={progressStyle}
+        size={200}
+        width={10}
+        fill={fill}
+        rotation={0}
+        tintColor={green}
+        backgroundColor={white}
+      >
+        {() => (
+          <View style={viewStyle}>
+            <Text style={textSeconds}>{timer}</Text>
+            <Text style={textSecondName}>{bottomText}</Text>
+          </View>
+        )}
+      </AnimatedCircularProgress>
+    )
+  }
+
+  const renderCircularView = () => {
+    return (
+      <View style={circularView}>
+        <FontAwesome name={image} size={30} color={white} />
+        <Text style={textSecondName}>{bottomText}</Text>
+      </View>
+    )
+  }
+
+  return (
+    <View style={container}>


This is something we've repeatedly asked not to have in the code. I think Vinod can maybe apply his learning from the SettingCell here, can he support you on this?

Bhuvneshwar-MB · 2020-05-01T14:02:47Z

can you please review now?

MDHayes self-requested a review April 30, 2020 14:05

MDHayes requested changes Apr 30, 2020

View reviewed changes

progress timer and installed vector icons and Timer components and wo…

618c911

…rked on suggestions

Bhuvneshwar-MB requested a review from MDHayes May 1, 2020 06:02

Use only FontAwesome for icons

b34aee7

MDHayes requested changes May 1, 2020

View reviewed changes

Bhuvneshwar-MB added 2 commits May 1, 2020 14:07

Add FontAwesome in Info.plist

0c55775

use same Button Component for Home Screen and Timer screen

e7f0401

Bhuvneshwar-MB added 3 commits May 1, 2020 16:03

conflict problem

ceeed9d

Resolve Conflicts

2679da8

Resolved conflicts

2aabd3f

Bhuvneshwar-MB requested a review from MDHayes May 1, 2020 11:10

vinod20191992 and others added 2 commits May 1, 2020 16:44

Linking and action reducers completed

eb1ef51

Merge pull request #1 from vinod20191992/feature/settings-screen-actions

c79787f

Feature/settings screen actions

MDHayes requested changes May 1, 2020

View reviewed changes

Bhuvneshwar-MB added 2 commits May 1, 2020 19:03

pull from develop

fe87a7f

Chnage in Timer Button

13a6238

Bhuvneshwar-MB requested a review from MDHayes May 1, 2020 14:02

Feature/feature timer component #9

Are you sure you want to change the base?

Feature/feature timer component #9

Uh oh!

Conversation

Bhuvneshwar-MB commented Apr 30, 2020

Uh oh!

MDHayes left a comment

Choose a reason for hiding this comment

Uh oh!

Bhuvneshwar-MB commented May 1, 2020

Uh oh!

Bhuvneshwar-MB commented May 1, 2020

Uh oh!

MDHayes left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MDHayes commented May 1, 2020

Uh oh!

Bhuvneshwar-MB commented May 1, 2020

Uh oh!

Bhuvneshwar-MB commented May 1, 2020

Uh oh!

MDHayes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bhuvneshwar-MB commented May 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MDHayes left a comment •

edited

Loading