Skip to content

Added common components #54

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

Merged
merged 14 commits into from
Jan 16, 2018
Merged

Added common components #54

merged 14 commits into from
Jan 16, 2018

Conversation

ignaciosantise
Copy link

Summary

  • Added Custom text input with showPassword prop
  • Added Loadable HOC to show a spinner when the data is loading
  • Added Custom button component
  • Updated eslint dependencies

Know Issues:

Screenshots

Loadable

Android

loadable android

iOS

loadable ios

Custom text input

Android

textinput android

iOS

textinput ios

@ignaciosantise
Copy link
Author


import styles from './styles';

export default class CustomButton extends PureComponent {
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep export default at the bottom of this file

customStyles = () =>
CustomButton.VARIANTS.map(variant => (this.props[variant] ? styles[variant] : null)).filter(
style => style !== null
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd extract this function and reuse it in customTextStyles

icon: PropTypes.number,
iconStyle: ViewPropTypes.style, // eslint-disable-line react/no-typos
onPress: PropTypes.func.isRequired,
style: ViewPropTypes.style, // eslint-disable-line react/no-typos
Copy link
Contributor

Choose a reason for hiding this comment

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

If applies, I'd delete this rule. What do you think ? @wfolini @sbalay @IgnacioAbadie @ignaciosantise

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

backgroundColor: transparent
};

export default StyleSheet.create({
Copy link
Contributor

Choose a reason for hiding this comment

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

export default at the bottom

Copy link
Contributor

Choose a reason for hiding this comment

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

i think that we have all export default of styles file like @ignaciosantise wrote.
In components maybe, but in style files i like this. Really there is not difference

Copy link
Author

@ignaciosantise ignaciosantise Jan 5, 2018

Choose a reason for hiding this comment

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

@mvbattan if we are changing the export position of Stylesheets, we should change all of them in another PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, let's do it in another PR

.filter(style => style !== null);
CustomText.VARIANTS.map(variant => (this.props[variant] ? styles[variant] : null)).filter(
style => style !== null
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd reuse the function extracted from CustomButton

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

display: 'flex',
flexDirection: 'row',
alignItems: 'center',
bottom: -4
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm, why this ? This doesn't have issues with Android overflow ?
If so, I'd put marginBottom = -4

<View
style={[
this.props.multiline ? styles.multilineContainer : styles.container,
this.props.bottomBorder ? styles.bottomBorder : {},
Copy link
Contributor

Choose a reason for hiding this comment

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

this.props.bottomBorder && styles.bottomBorder

?

Copy link
Author

Choose a reason for hiding this comment

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

Yeeep!

multiline: PropTypes.bool,
placeholder: PropTypes.string,
titleStyles: Text.propTypes.style, // eslint-disable-line react/no-typos
textStyles: Text.propTypes.style, // eslint-disable-line react/no-typos
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here.
Mmm isn't this deprecated ? 🤔 I've searched for TextPropTypes but I've found nothing.
For example, View.propTypes.style is deprecated.

Copy link
Author

Choose a reason for hiding this comment

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

View.propTypes.style is deprecated, but there's no info about Text.propTypes. and it doesnt throw a warning or sth


import { gray, black, transparent } from '../../../constants/colors';

export default StyleSheet.create({
Copy link
Contributor

Choose a reason for hiding this comment

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

export default


import { white } from '../../../constants/colors';

export default StyleSheet.create({
Copy link
Contributor

Choose a reason for hiding this comment

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

export default

@mvbattan mvbattan assigned ignaciosantise and unassigned mvbattan Jan 4, 2018
@IgnacioAbadie
Copy link
Contributor

@ignaciosantise check the conflics

@IgnacioAbadie IgnacioAbadie removed their assignment Jan 5, 2018
Copy link
Contributor

@wfolini wfolini left a comment

Choose a reason for hiding this comment

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

Excellent! some comments that I would like to say
I'd use this icons for login text inputs. They are more beautiful and without color
https://material.io/icons/#ic_visibility
https://material.io/icons/#ic_visibility_off

Also, I'd like us to change the way we pass the props to the CustomText component and maybe also CustomButton.
I'd only declare h1, h2, h3, p, etc props, and for that props declare the font family, color, size, weights, etc following the typesetting styles defined by the design department, like this example...

captura de pantalla 2018-01-08 a la s 10 12 04

What u think about this? @ignaciosantise @IgnacioAbadie @mvbattan @sbalay
I think that in this way the app design is going to be more neat and with less possibility of design issues.

@wfolini wfolini removed their assignment Jan 8, 2018
@ignaciosantise
Copy link
Author

@wfolini great! Assets changed ;)

About the props, think that configuration will depend on the project. Not all projects follow that design line. I think that colors and sizes should be customizable by props

@wfolini wfolini removed their assignment Jan 16, 2018
}/[email protected]`;
module.exports.CUSTOM_TEXT_INPUT_ASSETS_VISIBILITY_3X = `${
module.exports.CUSTOM_TEXT_INPUT_ASSETS_PATH
}/[email protected]`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Dunno. It's likely this ic_visibility is being repeated over and over

@mvbattan mvbattan removed their assignment Jan 16, 2018
@ignaciosantise ignaciosantise merged commit df7effe into master Jan 16, 2018
@ignaciosantise ignaciosantise deleted the common-components branch January 16, 2018 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants