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

[TextField / Labelled] Add character count #709

Merged
merged 1 commit into from Jan 29, 2019
Merged

Conversation

jfangrad
Copy link
Contributor

@jfangrad jfangrad commented Dec 3, 2018

WHY are these changes introduced?

Resolves #491

TextField lacks the ability to display the remaining characters available when the TextField has a max character length. This PR adds that ability.

WHAT is this pull request doing?

Adds in the prop showCharacterCount to TextField to enable the displaying of the remaining character count.

The Labelled component will then display the number of characters out of the total allowed characters that have been inputted into the TextField.

screen shot 2018-12-03 at 4 21 40 pm

One item of note is that things can get a little busy as can be seen below:

screen shot 2018-11-29 at 4 20 26 pm

@dpersing was giving insight into this on the issue.

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} from '@shopify/polaris';

interface State {
  value: string;
}

export default class Playground extends React.Component<{}, State> {
  state = {
    value: '',
  };

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

  render() {
    const {value} = this.state;
    return (
      <Page
        title="Playground"
        primaryAction={{content: 'View Examples', url: '/examples'}}
      >
        <TextField
          label="This is where the label lives"
          value={value}
          onChange={this.onChange}
          maxLength={10}
          showCharacterCount
        />
        <TextField
          label="This is where the label lives"
          value={value}
          onChange={this.onChange}
          maxLength={10}
          multiline         
          showCharacterCount
        />
      </Page>
    );
  }
}

🎩 checklist

@BPScott BPScott temporarily deployed to polaris-react-pr-709 December 3, 2018 21:49 Inactive
@jfangrad jfangrad self-assigned this Dec 12, 2018
src/components/Labelled/Labelled.tsx Outdated Show resolved Hide resolved
src/components/Labelled/Labelled.tsx Outdated Show resolved Hide resolved
src/components/Labelled/Labelled.tsx Outdated Show resolved Hide resolved
src/components/Labelled/Labelled.tsx Outdated Show resolved Hide resolved
src/components/TextField/tests/TextField.test.tsx Outdated Show resolved Hide resolved
@solonaarmstrong-zz
Copy link

solonaarmstrong-zz commented Dec 14, 2018

I wonder if it would make more sense to position the text count here:
screen shot 2018-12-14 at 7 03 08 am

This is how it is already implemented across the admin. It looks like it's using subdued text style as a sibling to the label.

@danrosenthal
Copy link
Contributor

I'm also in favor of that positioning, @solonaarmstrong. cc: @sarahill to confirm

@jfangrad
Copy link
Contributor Author

@solonaarmstrong @danrosenthal I also agree and think that it looks better in that position. The one reason, that came up in the issue, for putting it below was that if the textfield had a lot of text in it the user might not be able to see the count as they type if it was at the top of the field.

Its probably a rare case but just thought I would bring that up again to be included in the consideration.

@danrosenthal
Copy link
Contributor

The one reason, that came up in the issue, for putting it below was that if the textfield had a lot of text in it the user might not be able to see the count as they type if it was at the top of the field.

I think that's an acceptable limitation.

@sarahill
Copy link
Contributor

sarahill commented Dec 14, 2018

I like @solonaarmstrong suggestion for placement.

Couple questions/thoughts:

  • How would this look on smaller screens?
    • We could either not show the count on or reduce it to just the count 4 / 70.
  • Do we need to worry about a long label that could potentially run into the character count?
    • For this we could set a max-width and allow the label to wrap if necessary and base align the label + character count.

@solonaarmstrong-zz
Copy link

@sarahill This is how it currently behaves on small screens in the admin (for SEO, as an example). It simply needs to stack.

screen shot 2018-12-14 at 9 07 37 am

@BPScott BPScott temporarily deployed to polaris-react-pr-709 December 17, 2018 17:39 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-709 December 17, 2018 17:43 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-709 December 17, 2018 18:06 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-709 December 17, 2018 18:35 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-709 December 17, 2018 18:47 Inactive
@jfangrad
Copy link
Contributor Author

@danrosenthal @solonaarmstrong I made the changes to the placement etc.

One thing that I ran into is that the placement of the label action gets a little weird when also showing the character count. Currently this is what I have it doing when both are present:
screen shot 2018-12-17 at 1 34 08 pm

Its probably a super rare case to have all three...
To have the label action always be on the far right I would have to move the character count code down another level into the Label component which wouldn't be super nice.

label action will still be on the far right if the showCharacterCount is false

Copy link
Contributor

@danrosenthal danrosenthal left a comment

Choose a reason for hiding this comment

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

This is feeling like a really strong step in the right direction, but I feel that the separation of concerns is wrong here. The character count feels like it should be the concern of the TextField component itself, but in this implementation the Labelled component is doing most of the work. In my opinion, the generation of the characterCount markup should be the concern of the TextField itself, and should just be passed as a ReactNode to the Labelled component, which should not really care what it is, and only should be concerned with where to render it.

What do you think about doing all the work in the TextField component, and then passing it to the action prop of Labelled? Then Labelled could check if it received an object or a ReactNode as an action, and render it accordingly. My only hesitation here is that it would feel a bit unnatural to pass something that wasn't an Action to the action prop, but I think I'm okay with that. It would also solve your display problem of figuring out whether or not to render the Action, as the Labelled could only take one or the other.

Thoughts about this approach?

@danrosenthal danrosenthal dismissed their stale review December 18, 2018 15:04

Feedback addressed

@jfangrad
Copy link
Contributor Author

@danrosenthal I definitely agree with you. It felt weird putting all the logic in Labelled.

I like the idea of passing it through the action prop, my only concern is that you will then not be able to have both at the same time. I know for my project that isn't a concern. I guess if it becomes a problem in the future that can be addressed then?

My main question then is if someone passes a labelAction prop and the showCharacterCount prop to the TextField which should we pass to the Labelled components action prop? Which takes precedence? My initial thought is action should probably take precedence since it has been around longer.

I am also worried about people trying to do an action and a character count and wondering why one of them doesn't show up (But maybe just saying why in the docs is enough?)

@BPScott BPScott temporarily deployed to polaris-react-pr-709 January 24, 2019 20:21 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-709 January 24, 2019 21:47 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-709 January 24, 2019 21:53 Inactive
@ghost ghost removed the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Jan 25, 2019
@BPScott BPScott temporarily deployed to polaris-react-pr-709 January 25, 2019 15:06 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-709 January 25, 2019 15:09 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-709 January 28, 2019 18:40 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-709 January 28, 2019 18:51 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-709 January 28, 2019 18:56 Inactive
@emarchak
Copy link
Contributor

It ended up making more sense to provide a fixed line-height to the CharacterCount instead of shifting around the margins if it's a multiline text field. I believe we still need the .AlignFieldBottom since the layout had inconsistent behaviour when I applied align-self: flex-end; to a single line input field.

image

I believe this should be good to go pending the two ✅ and someone approving percy. @solonaarmstrong and @dpersing could you please take another look?

@dpersing
Copy link
Contributor

Hi @emarchak! It looks like the aria-label for the "X of Y" content is reading out "X characters remaining", which is the opposite of the visual. For example, when 0 characters have been typed, it says "0 characters remaining", whereas really it should be "10 characters remaining" since none have been typed yet:

<div id="TextField1CharacterCounter" class="TextField-CharacterCount_7V_ks" aria-label="0 characters remaining" aria-live="polite" aria-atomic="true">0/10</div>

I'm thinking that "X of Y characters used" would make more sense with the visual?

@BPScott BPScott temporarily deployed to polaris-react-pr-709 January 28, 2019 20:01 Inactive
@emarchak
Copy link
Contributor

Fixed!


"TextField": {
"characterCount": "{count} characters",
"characterCountWithMaxLength": "{count} of {limit} characters used"
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@emarchak emarchak merged commit a3ac46d into master Jan 29, 2019
@ghost
Copy link

ghost commented Jan 29, 2019

🎉 Thanks for your contribution to Polaris React!

@fchienvuhoang
Copy link

fchienvuhoang commented Feb 21, 2019

I'm using bellow code example but showCharacterCount not work, my Polaris v3.8.0, any update?

 <TextField
        label="Store name"
        value={this.state.value}
        onChange={this.handleChange}
        maxLength={20}
        showCharacterCount
        multiline
      />

@solonaarmstrong-zz
Copy link

solonaarmstrong-zz commented Feb 22, 2019

@fchienvuhoang I just tried and I see it in there in v3.9.0.

screen shot 2019-02-22 at 9 31 15 am

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.

None yet