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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TextField] Move character counter to bottom of multiline text input. #992

Merged
merged 1 commit into from Feb 12, 2019

Conversation

emarchak
Copy link
Contributor

@emarchak emarchak commented Feb 8, 2019

WHY are these changes introduced?

Resolves #986

Follow up on #709, we found that having the character count on the same row in a multiline textField caused strange wrapping on narrow inputs. This PR uses flex-wrap: wrap; on multiline fields to move the counter to the bottom of the input so we take up vertical instead of horizontal space.

WHAT is this pull request doing?

Before

image

After

image

How to 馃帺

馃枼 Local development instructions
馃棐 General tophatting guidelines
馃搫 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import * as React from 'react';
import {Page, TextField, FormLayout} from '@shopify/polaris';

interface State {
  value: string;
}

export default class Playground extends React.Component<{}, State> {
  state = {
    value: 'Quaerat exercitation fermentum omnis similique, sed semper blandit penatibus senectus sociosqu pulvinar, consequatur vel modi consequuntur facilis earum repellendus occaecati laborum, pariatur consequatur cubilia, incidunt eu eveniet consectetuer, tellus optio.\n\n' +
      'Platea quae ridiculus exercitationem sunt commodo rerum aliquam officia do eum quis consequatur netus minima auctor eleifend doloribus iure a mauris nulla accusamus aliquet, sem felis vel? Nostra minim earum.'
  };

  onChange = (value: string) => {
    this.setState({value});
  };

  render() {
    const {value} = this.state;
    const prefix = '1-800-';
    const help = 'Need more support? Call Batman.';
    const labelValue = "If there's something weird and it don't look good, who you gonna call?";
    return (
      <Page
        title="Playground"
        primaryAction={{content: 'View Examples', url: '/examples'}}
      ><FormLayout.Group condensed>
        <TextField
          label={labelValue}
          value={value}
          onChange={this.onChange}
          maxLength={800}
          prefix={prefix}
          helpText={help}
          showCharacterCount
        />
        <TextField
          label={labelValue}
          value={value}
          onChange={this.onChange}
          maxLength={800}
          helpText={help}
          multiline
          showCharacterCount
        />
        <TextField
          label={labelValue}
          value={value}
          onChange={this.onChange}
          helpText={help}
          multiline
          showCharacterCount
        />
        <TextField
          label={labelValue}
          value={value}
          onChange={this.onChange}
          helpText={help}
          multiline
        />
      </FormLayout.Group></Page>
    );
  }
}

馃帺 checklist

@ghost
Copy link

ghost commented Feb 8, 2019

馃憢 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven鈥檛 already, please check out the contributing guidelines. You can also join #polaris on the Shopify Partners Slack.

@BPScott BPScott temporarily deployed to polaris-react-pr-992 February 8, 2019 22:22 Inactive
@BPScott BPScott requested a deployment to polaris-react-pr-992 February 8, 2019 22:22 Abandoned
@BPScott BPScott requested a deployment to polaris-react-pr-992 February 8, 2019 22:22 Abandoned
@BPScott BPScott requested a deployment to polaris-react-pr-992 February 8, 2019 22:23 Abandoned
@BPScott BPScott requested a deployment to polaris-react-pr-992 February 8, 2019 22:25 Abandoned
@BPScott BPScott requested a deployment to polaris-react-pr-992 February 8, 2019 22:28 Abandoned
@BPScott BPScott requested a deployment to polaris-react-pr-992 February 8, 2019 22:34 Abandoned
@BPScott BPScott temporarily deployed to polaris-react-pr-992 February 8, 2019 22:38 Inactive
Copy link
Contributor

@dpersing dpersing left a comment

Choose a reason for hiding this comment

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

Looks fantastic to me (including with text-only resize!). Thank you @emarchak! 馃帀

@BPScott BPScott temporarily deployed to polaris-react-pr-992 February 10, 2019 01:16 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-992 February 10, 2019 01:19 Inactive
@tmlayton
Copy link
Contributor

My preference is for multiline with a character count to still be able to be a single line:

I pushed changes to absolute position the count when it is multiline, gave it a background color, and a pill shape. The positioning is calculated so it is vertically centered to the control height line.

changes

@emarchak
Copy link
Contributor Author

emarchak commented Feb 11, 2019

@tmlayton While it looks better as the initial state, in the GIF the pill covers up some of the text that you're writing. Users will have to add a line break to see what words they're typing.

That seems to me to be a net negative change, and it wouldn't be appropriate for our use case as we have a text limit of 2800 which would take up most of the space.

@elliejeon what are your thoughts on this?

@elliejeon
Copy link

I agree with @emarchak , text being covered by the counter is not the best experience.
Can we have the counter on a separate line for multiline textfields?

@tmlayton
Copy link
Contributor

While it looks better as the initial state

It is not that it looks better, adding a second line is a visual breaking change and could potentially break a consumer鈥檚 layout. I don鈥檛 like that it covers text either, but was trying to solve without JS.

@sarahill @ry5n thoughts on this? If we are okay with the change in initial visual state (being a minimum of 2 lines) then feel free to revert my commit.

@ry5n
Copy link
Contributor

ry5n commented Feb 11, 2019

I don鈥檛 think we should cover the user鈥檚 input with the count.

It鈥檚 definitely important to minimize breaking visual and interaction changes (though we don鈥檛 treat them as strictly as breaking API changes). In this case I鈥檓 comfortable with the change because if the developer is choosing a multiline field, they should be taking into account the field being able to grow vertically. (The only counter-example I can think of is if they鈥檝e also specified a maxlength, but given how recent this feature is, I think the risk is probably low.)

Based on the images, it looks like the field remains on one line for non-multiline fields, and multiline fields with a counter are always at least 2 lines. I鈥檓 OK with this 馃憤

@emarchak
Copy link
Contributor Author

emarchak commented Feb 12, 2019

adding a second line is a visual breaking change and could potentially break a consumer鈥檚 layout.

Good point. I didn't think to consider that!

Given the above discussion, I'm going to revert @tmlayton's commit and then merge this in when the build passes. Thanks to everyone for your input. 馃檹

@ry5n -- The non-multiline version still remains a single line and we use the standard browser behaviour to let the users navigate left and right on the input fo the text. For visibility, here's what it looks like when I type out text into an empty field:

kapture 2019-02-12 at 10 00 05

@ghost
Copy link

ghost commented Feb 12, 2019

馃帀 Thanks for your contribution to Polaris React!

@emarchak emarchak deleted the character-count-textarea-bug branch February 12, 2019 16:48
@tmlayton
Copy link
Contributor

Thanks for this @emarchak!

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.

Character limit component
6 participants