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] Add characterCount attribute #3540

Closed
wants to merge 1 commit into from

Conversation

heetvachhani
Copy link
Contributor

This is my first draft. I will add example once you guys go through this.
Thanks!!

References #2416

@nathanmarks nathanmarks added PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Feb 29, 2016
@nathanmarks
Copy link
Member

@heetvachhani I haven't reviewed the changes, but looks like some linter errors have cropped up, should be a quick fix.

@nathanmarks nathanmarks removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Feb 29, 2016
@@ -296,6 +312,9 @@ const TextField = React.createClass({
hasValue: isValid(props.value) || isValid(props.defaultValue) ||
(props.valueLink && isValid(props.valueLink.value)),
muiTheme: this.context.muiTheme || getMuiTheme(),
currentCharCount: 0,
Copy link
Member

Choose a reason for hiding this comment

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

this should be char count of defaultValue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do the change

@heetvachhani
Copy link
Contributor Author

@alitaheri I'm not able to put 'currentCharCount' into default value as it's state not props
also shall I create new example file or will add into existing one?

output_4cqwgm

@alitaheri
Copy link
Member

@heetvachhani What I mean is, if you have foo as defaultValue passed down from props, then the char count should initially be 3, not 0.

@heetvachhani
Copy link
Contributor Author

oh got your point @alitaheri. Thanks!! 👍 didn't thought of that case 😅

@alitaheri
Copy link
Member

Oh, I almost forgot about value too 😆 watch out for that one too 😆

@heetvachhani
Copy link
Contributor Author

@alitaheri i guess it will work now 😄

@heetvachhani heetvachhani force-pushed the count_character branch 3 times, most recently from 3936295 to 7d8611f Compare March 1, 2016 23:01
/**
* Maximum number of character in the Textfield
*/
charCountMax: React.PropTypes.number,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just consider using the standard html maxlength attribute?

Copy link
Member

Choose a reason for hiding this comment

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

I think maxlength would block the input from going over the max character count, which can be exceeded with a warning for the character counter in the material spec.

@mbrookes mbrookes added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Mar 2, 2016
};

const defaultProps = {
};
Copy link
Member

Choose a reason for hiding this comment

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

We can cut this out if not using any defaultProps 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nathanmarks yes will remove it.

@nathanmarks nathanmarks added on hold There is a blocker, we need to wait and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Mar 2, 2016
@nathanmarks
Copy link
Member

@newoga I set this to on-hold while we figure out a couple of the questions surrounding TextField

@mbrookes mbrookes changed the title [TextField] added characterCount attribute [TextField] Add characterCount attribute Mar 3, 2016
@mbrookes mbrookes added the new feature New feature or request label Mar 5, 2016
@nathanmarks nathanmarks added this to the 0.16.0 Release milestone Mar 5, 2016
@nathanmarks
Copy link
Member

@mbrookes As it stands -- this implementation is not making it into the lib. Can we close in favour of a new PR once the TextField has been refactored?

@mbrookes
Copy link
Member

mbrookes commented Mar 7, 2016

I agree.

@heetvachhani, sorry you caught another one of these!

@mbrookes mbrookes closed this Mar 7, 2016
@oliviertassinari oliviertassinari removed the on hold There is a blocker, we need to wait label Mar 19, 2016
@dcworldwide
Copy link

Was this ever implemented? I have the same requirement for my new customer. Seems like a obvious addition

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants