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

EuiRange - onValidatedChange props #1461

Closed
wants to merge 4 commits into from

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jan 20, 2019

fixes #1458

Adds prop onValidatedChange to EuiRange. onValidatedChange is only called when value changes to a Number that is within the provided min and max (inclusive).

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Thanks for adding this, I just saw a couple of issues and had a couple of suggestions.

Also, could you do one more change to support decimals in the sizing of the attached input by changing that line to:

const maxWidthStyle = { maxWidth: `${Math.max(String(min + step).length, String(max + step).length) + 2}em` };

src-docs/src/views/form_controls/range.js Show resolved Hide resolved
{this.renderLabel('max')}
{extraInputNode}
</div>
{this.renderErrorMessage()}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should leave out rendering the error message in the DOM because this should technically be handled by a EuiFormRow that hooks up the message to the input. Currently this error message does not get read by a screen reader as it is.

Maybe it should just pass back an error message then the consumer can hook it into the error prop of EuiFormRow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I will add a onValidationError callback that receives the error message

src/components/form/range/range.js Outdated Show resolved Hide resolved
src/components/form/range/range.js Outdated Show resolved Hide resolved
@nreese
Copy link
Contributor Author

nreese commented Jan 23, 2019

@cchaos I am not sure I like having separate callbacks onValidatedChange and onValidationError. What do you think about only having onValidatedChange and it is called with isValid and value? That way the consumer can craft the error message accordingly.

@cchaos
Copy link
Contributor

cchaos commented Jan 23, 2019

I think that's fine. That'll also help with localization.

@thompsongl thompsongl mentioned this pull request Jan 28, 2019
10 tasks
@nreese
Copy link
Contributor Author

nreese commented Feb 4, 2019

closing. EuRange refactor has made these changes obsolete

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.

EuiRange show empty string in input when value is empty
2 participants