Skip to content

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’t 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
@emarchak emarchak force-pushed the character-count-textarea-bug branch from 496b21e to e1651aa Compare February 8, 2019 22:38
@BPScott BPScott temporarily deployed to polaris-react-pr-992 February 8, 2019 22:38 Inactive
@emarchak emarchak requested review from dpersing and tmlayton February 8, 2019 22:39
Copy link

@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
@tmlayton tmlayton force-pushed the character-count-textarea-bug branch from 988a77d to 010225f Compare February 10, 2019 01:19
@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’s layout. I don’t 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’t think we should cover the user’s input with the count.

It’s definitely important to minimize breaking visual and interaction changes (though we don’t treat them as strictly as breaking API changes). In this case I’m 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’ve 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’m 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’s 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