Skip to content

Support rendering leading Icon to TextField #815

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.

Sign up for GitHub

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 5 commits into from
Jun 24, 2020
Merged

Support rendering leading Icon to TextField #815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2068d80
support rendering TextField leading Icon
ethanshar Jun 15, 2020
5b2b2a7
Don't allow rendering prefix or leadingIcon with floatingPlaceholder
ethanshar Jun 15, 2020
4d6f9f3
remove empty lines
ethanshar Jun 24, 2020
435ad70
Remove call for unimplemented onLeadingIconLayout
ethanshar Jun 24, 2020
e112379
remove tab
ethanshar Jun 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions demo/src/screens/componentScreens/TextFieldScreen/BasicTextFieldScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, {Component} from 'react';
import {ScrollView} from 'react-native';
import {Colors, View, Text, TextField, Slider, ColorPalette} from 'react-native-ui-lib'; //eslint-disable-line
import {Assets, Spacings, View, Text, TextField} from 'react-native-ui-lib'; //eslint-disable-line

import {
renderBooleanOption,
Expand All @@ -21,13 +21,19 @@ const GUIDING_TEXTS = {
floatingPlaceholder: 'Floating Placeholder'
};

const LEADING_ICON = {
source: Assets.icons.demo.search,
style: {marginRight: Spacings.s1}
};
Copy link
Contributor

@ArnonZ ArnonZ Jun 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We don't want to supply a default margin and let it be overridden?
This forces to always supply a margin since it doesn't look good without it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I preferred to keep it clean as long as it doesn't look bad without custom styling.
This spacing is just for the sake of this example.
I assume we will pass something else in the private config


export default class BasicTextFieldScreen extends Component {
constructor(props) {
super(props);

this.state = {
hideUnderline: false,
withPrefix: false,
withLeadingIcon: false,
underlineColor: undefined,
guidingText: GUIDING_TEXTS.none,
disabled: false,
Expand All @@ -45,6 +51,7 @@ export default class BasicTextFieldScreen extends Component {
const {
hideUnderline,
withPrefix,
withLeadingIcon,
underlineColor,
guidingText,
titleColor,
Expand All @@ -67,6 +74,7 @@ export default class BasicTextFieldScreen extends Component {
hideUnderline={hideUnderline}
underlineColor={underlineColor}
prefix={withPrefix ? 'prefix://' : undefined}
leadingIcon={withLeadingIcon ? LEADING_ICON : undefined}
title={guidingText === GUIDING_TEXTS.useTitle ? 'Title' : undefined}
titleColor={titleColor}
floatingPlaceholder={guidingText === GUIDING_TEXTS.floatingPlaceholder}
Expand Down Expand Up @@ -98,6 +106,7 @@ export default class BasicTextFieldScreen extends Component {
{renderBooleanOption.call(this, 'Centered', 'centered')}
{renderBooleanOption.call(this, 'Hide Underline', 'hideUnderline')}
{renderBooleanOption.call(this, 'With Prefix', 'withPrefix')}
{renderBooleanOption.call(this, 'With leadingIcon', 'withLeadingIcon')}
{renderColorOption.call(this, 'Underline Color', 'underlineColor')}
{renderRadioGroup.call(this, 'Guiding Text', 'guidingText', GUIDING_TEXTS)}
{renderColorOption.call(this, 'Title Color', 'titleColor')}
Expand All @@ -109,4 +118,3 @@ export default class BasicTextFieldScreen extends Component {
);
}
}

22 changes: 15 additions & 7 deletions src/components/inputs/TextField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ export default class TextField extends BaseInput {
*/
transformer: PropTypes.func,
/**
* Pass to render a prefix text as part of the input
* Pass to render a prefix text as part of the input (doesn't work with floatingPlaceholder)
*/
prefix: PropTypes.string,
/**
Expand Down Expand Up @@ -172,7 +172,11 @@ export default class TextField extends BaseInput {
iconColor: PropTypes.string,
onPress: PropTypes.func,
style: PropTypes.oneOfType([PropTypes.object, PropTypes.number])
})
}),
/**
* Pass to render a leading icon to the TextInput value. Accepts Image props (doesn't work with floatingPlaceholder)
*/
leadingIcon: PropTypes.shape(Image.propTypes)
};

static defaultProps = {
Expand Down Expand Up @@ -310,8 +314,7 @@ export default class TextField extends BaseInput {
}

getTopPaddings() {
const {floatingPlaceholder} = this.getThemeProps();
return floatingPlaceholder ? (this.shouldShowTopError() ? undefined : 25) : undefined;
return this.shouldFakePlaceholder() ? (this.shouldShowTopError() ? undefined : 25) : undefined;
}

isDisabled() {
Expand Down Expand Up @@ -345,8 +348,9 @@ export default class TextField extends BaseInput {
}

shouldFakePlaceholder() {
const {floatingPlaceholder, centered} = this.getThemeProps();
return Boolean(floatingPlaceholder && !centered && !this.shouldShowTopError());
const {floatingPlaceholder, centered, leadingIcon, prefix} = this.getThemeProps();

return !leadingIcon && !prefix && Boolean(floatingPlaceholder && !centered && !this.shouldShowTopError());
Copy link
Contributor

@ArnonZ ArnonZ Jun 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why not to allow leading icon and floatingPlaceholder?. Missing onLayout implementation for this of course (See relevant comment above).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was about to add it, but it caused a weird UI issue where the floating placeholder transition looked bad.
Anyway, I discussed with Yulia and she agreed there's no reason for both of these features to come together.

}

shouldShowError() {
Expand Down Expand Up @@ -389,6 +393,9 @@ export default class TextField extends BaseInput {
style={[
this.styles.placeholder,
typography,
// TODO: we need to exclude completely any dependency on line height
// in this component since it always breaks alignments
{lineHeight: undefined},
{
transform: [
{
Expand Down Expand Up @@ -627,7 +634,7 @@ export default class TextField extends BaseInput {
}

render() {
const {expandable, containerStyle, underlineColor, useTopErrors, hideUnderline} = this.getThemeProps();
const {expandable, containerStyle, underlineColor, useTopErrors, hideUnderline, leadingIcon} = this.getThemeProps();
const underlineStateColor = this.getStateColor(underlineColor || UNDERLINE_COLOR_BY_STATE);

return (
Expand All @@ -642,6 +649,7 @@ export default class TextField extends BaseInput {
{paddingTop: this.getTopPaddings()}
]}
>
{leadingIcon && <Image {...leadingIcon}/>}
{this.renderPrefix()}
{this.renderPlaceholder()}
{expandable ? this.renderExpandableInput() : this.renderTextInput()}
Expand Down