Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

issue #104 resolved #116

Merged

Conversation

shubhamkakkar
Copy link
Contributor

Converted all class based component into react hooks
PS: yarn test -> failed // jest not found !!

src/index.js Outdated
import theme, { withGalio, GalioProvider } from './theme';

import galioConfig from './config/galio.json';
import galioConfig from './fonts/galio.json';
Copy link
Collaborator

Choose a reason for hiding this comment

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

As in v0.5.4 we're using ./config/galio.json which solved an issue regarding deploying apps with Galio for Android. Check #104

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh!!! changing that !!!

@@ -1,40 +1,40 @@
import React, { Component } from 'react';
import React from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

This same file exists in PR #115 . We need to only accept one of them, please take a look at my comments at #115. Should we close the other one and only pick this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, close the earlier !!

src/Radio.js Outdated
@@ -2,21 +2,14 @@ import React from 'react';
import { View, TouchableOpacity, StyleSheet } from 'react-native';
import PropTypes from 'prop-types';
// G A L I O - D E P E N D E N C Y
import { Text } from '.';
import { Text } from 'galio-framework';
Copy link
Collaborator

Choose a reason for hiding this comment

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

The import should've stayed as import { Text } from './';. Galio is not a dependency of Galio and relative paths are better for users when they're importing our library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

src/Radio.js Outdated
this.spaceAround = this.spaceAround.bind(this);
}

function Radio(props) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've noticed the fact that you were most of the times destructuring the props object so you could use the variables inside the function, you could try and do the same thing here so you won't have to always do that for every function inside this Hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool !!, will do that!

src/NavBar.js Outdated
@@ -3,14 +3,14 @@ import { View, TouchableOpacity, StyleSheet, Dimensions } from 'react-native';
import PropTypes from 'prop-types';

// galio components
import { Block, Text, Icon } from '.';
import { Block, Text, Icon } from 'galio-framework';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same this as with the Radio component: ... from './' .

src/Card.js Outdated
import { Image, StyleSheet } from 'react-native';
import PropTypes from 'prop-types';

import { Block, Icon, Text } from '.';
import { Block, Icon, Text } from 'galio-framework';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing as with the other imports, they should be relative.

src/Button.js Outdated
@@ -2,13 +2,13 @@ import React from 'react';
import { ActivityIndicator, Dimensions, StyleSheet, TouchableOpacity, Text } from 'react-native';
import PropTypes from 'prop-types';
// galio components
import { Icon } from '.';
import { Icon } from 'galio-framework';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Imports should be relative.

Example: .. from './'.

src/Block.js Outdated
...props
} = this.props;
function Block(props) {
const {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should try destructuring the props in the props argument of the function. Like in Text.js

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 have actually destructed the props!
like this ( Pattern A )

const {
    row,
    flex,
    center,
    middle,
    top,
    bottom,
    right,
    left,
    shadow,
    space,
    fluid,
    height,
    shadowColor,
    card,
    width,
    safe,
    children,
    style,
    styles,
    ...rest
  } = props;

but not like this, ( Pattern B )

function Block({
    row,
    flex,
    center,
    middle,
    top,
    bottom,
    right,
    left,
    shadow,
    space,
    fluid,
    height,
    shadowColor,
    card,
    width,
    safe,
    children,
    style,
    styles,
    ...rest
  })

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 have used both pattern A and pattern B in the code !
If you could tell the particulat pattern you want the PR to have for the code consistency

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like the pattern B could be better but we could leave it like that for the moment.

src/Block.js Outdated
);
}

return (
<View {...rest} style={styleBlock}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

place the {...rest} at the end of the component just like this: <View style={styleBlock} {...rest}>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done !!

.idea/.gitignore Outdated
@@ -0,0 +1,3 @@

# Default ignored files
/workspace.xml
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what's the thing with the workspace.xml and the `.idea. folder.

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 will take a look and most probably remove this!

@shubhamkakkar shubhamkakkar mentioned this pull request Aug 21, 2019
shubhamkakkar pushed a commit to shubhamkakkar/galio that referenced this pull request Aug 21, 2019
shubhamkakkar pushed a commit to shubhamkakkar/galio that referenced this pull request Aug 21, 2019
@shubhamkakkar shubhamkakkar deleted the master-shubhamKakkar-issue#104 branch August 21, 2019 07:42
@shubhamkakkar
Copy link
Contributor Author

Sorry !! I accidentally deleted the branch!!
( Something is strangely wrong with me these days :-p

Copy link
Collaborator

@palingheorghe palingheorghe left a comment

Choose a reason for hiding this comment

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

Everything is fine, I tested everything and it works please reply to my last comments and we're good to go with this PR.

Thanks!

src/Block.js Outdated
...props
} = this.props;
function Block(props) {
const {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like the pattern B could be better but we could leave it like that for the moment.

src/Checkbox.js Outdated
import GalioTheme, { withGalio } from './theme';

function Checkbox(props) {
const [checked, setchecked] = React.useState(props.initialValue);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This hasn't been changed.

src/Checkbox.js Outdated
function Checkbox(props) {
const [checked, setchecked] = React.useState(props.initialValue);
React.useEffect(() => {
props.onChange(checked);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please reply.

@shubhamkakkar
Copy link
Contributor Author

shubhamkakkar commented Aug 27, 2019

@palingheorghe
I missed it I suppose!!
I am sorry for the trouble, I will do it by tonight and also use pattern B all over !!!

@shubhamkakkar
Copy link
Contributor Author

@palingheorghe I have implemented pattern B, and resolved the checkout.js changes

Copy link
Collaborator

@palingheorghe palingheorghe left a comment

Choose a reason for hiding this comment

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

Only two changes left and we're good.

src/Icon.js Outdated
@@ -4,44 +4,42 @@ import PropTypes from 'prop-types';

import GalioTheme, { withGalio } from './theme';
import getIconType from './helpers/getIconType';
import galioConfig from './config/galio.json';
import galioConfig from './fonts/galio.json';
Copy link
Collaborator

@palingheorghe palingheorghe Aug 28, 2019

Choose a reason for hiding this comment

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

Please change the path here in ./config/galio.json.

src/Checkbox.js Outdated Show resolved Hide resolved
@palingheorghe palingheorghe changed the base branch from master to Alpha-1 September 5, 2019 12:50
@palingheorghe palingheorghe merged commit e487742 into galio-org:Alpha-1 Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants