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

[CircularProgress] Remove min & max props #11211

Merged
merged 6 commits into from
May 3, 2018
Merged

[CircularProgress] Remove min & max props #11211

merged 6 commits into from
May 3, 2018

Conversation

mbrookes
Copy link
Member

@mbrookes mbrookes commented May 2, 2018

Makes the API consistant with LinearProgress

Breaking change

<CircularProgress
- min={10}
- max={20}
- value={15}
+ value={(15 - 10) / (20 - 10) * 100}
/>

Makes the API consistant with LinearProgress
@oliviertassinari
Copy link
Member

@mbrookes Should we add a note about accessibility and document the upgrade path after this breaking change?

@mbrookes
Copy link
Member Author

mbrookes commented May 2, 2018

The upgrade path should be obvious to anyone capable of using the component, but sure, we can cover it in the release notes.

Not sure I understand on the accessibility question - it's already taken care of. (You don't need to specify aria-min and aria-max when their values are 0 and 100 respectively.)

@oliviertassinari
Copy link
Member

Not sure I understand on the accessibility question

@mbrookes I was wondering if adding a demo demonstrating the min/max use case + the accessibility correctly taken care of would be valuable.

@mbrookes
Copy link
Member Author

mbrookes commented May 2, 2018

I can add a demo for min / max if you really think it's needed, but there's isn't anything to demo for ARIA.

@oliviertassinari
Copy link
Member

I see some value in it. I don't think it's really needed.

@mbrookes
Copy link
Member Author

mbrookes commented May 2, 2018

I've added a quick demo by modifying the current CircularDeterminate demo. Let me know if you think it should be a separate demo (rather than complicate the basic one), or whether a note in the docs with the sample code:

const normalise = value => (value - MIN) * (MAX - MIN);

would be sufficient.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 2, 2018

@mbrookes I was thinking of a new demo. I don't think we need to complexify the existing ones 😁. So we would also demonstrate more advanced transformation like a logarithm scale #6611 (topic for a different pr)

@oliviertassinari
Copy link
Member

oliviertassinari commented May 2, 2018

or whether a note in the docs with the sample code:

Oh, it's even better! It sounds like the best of the two worlds :)

@oliviertassinari oliviertassinari merged commit b774b72 into mui:v1-beta May 3, 2018
@franklixuefei
Copy link
Contributor

@mbrookes You forgot to change the typescript definition :-D I'm on it now.

@mbrookes
Copy link
Member Author

mbrookes commented May 8, 2018

Thanks - I don’t use TS so always forget it.

@fauskanger
Copy link
Contributor

I think there was a typo or something in the example of a normalize-function in the docs. I've added a PR to fix it: #12066 .

@eps1lon eps1lon modified the milestones: v5, v4.x Aug 4, 2020
@oliviertassinari oliviertassinari added the component: progress This is the name of the generic UI component, not the React module! label Nov 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y breaking change component: CircularProgress The React component component: progress This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants