Skip to content

[CircularProgress] Remove min & max props#11211

Merged
oliviertassinari merged 6 commits into
mui:v1-betafrom
mbrookes:circularprogress-remove-min-max
May 3, 2018
Merged

[CircularProgress] Remove min & max props#11211
oliviertassinari merged 6 commits into
mui:v1-betafrom
mbrookes:circularprogress-remove-min-max

Conversation

@mbrookes

@mbrookes mbrookes commented May 2, 2018

Copy link
Copy Markdown
Member

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
@mbrookes mbrookes added breaking change Introduces changes that are not backward compatible. component: CircularProgress The React component. labels May 2, 2018
@oliviertassinari

Copy link
Copy Markdown
Member

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

@mbrookes

mbrookes commented May 2, 2018

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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

mbrookes commented May 2, 2018

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Member

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

@mbrookes

mbrookes commented May 2, 2018

Copy link
Copy Markdown
Member Author

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

oliviertassinari commented May 2, 2018

Copy link
Copy Markdown
Member

@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

oliviertassinari commented May 2, 2018

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor

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

@mbrookes

mbrookes commented May 8, 2018

Copy link
Copy Markdown
Member Author

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

@fauskanger

Copy link
Copy Markdown
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 scope: progress Changes related to the progress. 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 Introduces changes that are not backward compatible. component: CircularProgress The React component. scope: progress Changes related to the progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants