Skip to content

Conversation

AlexCLeduc
Copy link
Contributor

@AlexCLeduc AlexCLeduc commented Oct 22, 2019

WHY are these changes introduced?

Fixes #2282. I made a single PR because it was one issue, but maybe it should be split in two

WHAT is this pull request doing?

  1. Removes default props for max, min, step so they no longer show up in markup unless explicitly passed
  2. Prevents aria-invalid="false" from being rendered when there is no error

@ghost
Copy link

ghost commented Oct 22, 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.

@ghost ghost added the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Oct 22, 2019
@AlexCLeduc AlexCLeduc changed the title [TextField] delete irrelevant default numeric-range props, prevent aria-invalid='false' [WIP][TextField] delete irrelevant default numeric-range props, prevent aria-invalid='false' Oct 22, 2019
@AlexCLeduc AlexCLeduc force-pushed the textfield_remove_weird_default_props branch from 020cb8f to d83980e Compare October 22, 2019 12:37
@ghost ghost removed the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Oct 22, 2019
@AlexCLeduc AlexCLeduc force-pushed the textfield_remove_weird_default_props branch from 630c18e to 8fe58ce Compare October 27, 2019 20:28
@AlexCLeduc AlexCLeduc changed the title [WIP][TextField] delete irrelevant default numeric-range props, prevent aria-invalid='false' [TextField] delete irrelevant default numeric-range props, prevent unconditional aria-invalid='false' Oct 27, 2019
@AlexCLeduc AlexCLeduc marked this pull request as ready for review November 2, 2019 21:33
@danrosenthal danrosenthal requested a review from dleroux December 9, 2019 15:49
const normalizedStep = step != null ? step : 1;
const normalizedMax = max != null ? max : Infinity;
const normalizedMin = min != null ? min : -Infinity;

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

const normalizedMin = min != null ? min : -Infinity;

const {unstableGlobalTheming = false} = useFeatures();

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@dleroux dleroux self-requested a review December 16, 2019 13:48
@dleroux
Copy link
Contributor

dleroux commented Dec 16, 2019

Thank you for doing this @AlexCLeduc! There was a conflict I tried to resolve using Github but it introduced formatting issues in ci. Ping me when those are updated and I'll approve.

@AlexCLeduc AlexCLeduc force-pushed the textfield_remove_weird_default_props branch from ed7d359 to 7621636 Compare December 17, 2019 00:23
@AlexCLeduc
Copy link
Contributor Author

Thanks @dleroux! I've just rebased over master

Copy link
Contributor

@dleroux dleroux left a comment

Choose a reason for hiding this comment

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

🎉 Thanks agiain @AlexCLeduc

@dleroux dleroux merged commit dae598a into Shopify:master Dec 17, 2019
@ghost
Copy link

ghost commented Dec 17, 2019

🎉 Thanks for your contribution to Polaris React!

@dleroux dleroux temporarily deployed to production December 17, 2019 20:45 Inactive
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.

[TextField] min, max, step and aria-invalid="false" shouldn't appear in the markup

2 participants