Skip to content

[CircularProgress] Remove the thickness property#4972

Closed
oliviertassinari wants to merge 1 commit intomui:masterfrom
oliviertassinari:circular-progress-remove-thickness
Closed

[CircularProgress] Remove the thickness property#4972
oliviertassinari wants to merge 1 commit intomui:masterfrom
oliviertassinari:circular-progress-remove-thickness

Conversation

@oliviertassinari
Copy link
Copy Markdown
Member

We are taking advantage of the CSS animation on the next branch
to render the component. One of the main advantages is to make the
component works without a js runtime. That's quite useful when the
component is rendered server side.
However, that's making the implementation of a thickness more complex.
The material specification doesn't seem to use different thickness.
Hence, I think that it would be better to make the master branch closer
to the next one.

@oliviertassinari
Copy link
Copy Markdown
Member Author

Actually, I have founds an alternative on the next branch.

@oliviertassinari oliviertassinari deleted the circular-progress-remove-thickness branch August 12, 2016 19:05
@nathanmarks
Copy link
Copy Markdown
Contributor

nathanmarks commented Aug 12, 2016

@oliviertassinari what what what! How did you do it?

@oliviertassinari
Copy link
Copy Markdown
Member Author

@nathanmarks I feel like you are not going to like it, I havn't changed the CSS generated, just the svg properties. The result is not perfect but probably enough for people tweaking around 😁. Well your call, we can revert.

@nathanmarks
Copy link
Copy Markdown
Contributor

@oliviertassinari yeah, not sure about it, it feels like a bug waiting to be reported IMO

@oliviertassinari
Copy link
Copy Markdown
Member Author

OK, I'm gonna spend some time on the component to see if I can find a better approach.

@nathanmarks
Copy link
Copy Markdown
Contributor

@oliviertassinari I think I know what needs to be done, ping me on gitter.

@oliviertassinari
Copy link
Copy Markdown
Member Author

oliviertassinari commented Aug 13, 2016

ping me on gitter

The overall conclusion is that it requires more time to be solved. Time that we think would be better spent on other issues.

Comment thread scripts/run-travis-tests.sh Outdated
npm run test:coverage
npm run test:karma
coveralls < ./test/coverage/lcov.info
cat ./test/coverage/lcov.info | ./node_modules/coveralls/bin/coveralls.js
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Revert #4941 as seems to be linked to some flakiness with the coverage fails.

We are taking advantage of the CSS animation on the next branch
to render the component. One of the main advantage is to make the
component works without a js runtime. That's quite useful when the
component is rendered server side.
However, that's making the implementation of a thickness more complex.
The material specification doesn't seems to use different thickness.
Hence, I think that it would be better to make the master branch closer
to the next one.
@oliviertassinari oliviertassinari added this to the v0.16.0 Release milestone Aug 14, 2016
@oliviertassinari
Copy link
Copy Markdown
Member Author

Turn out, we also need to generate dynamic classes to work with Safari and different size.
I'm closing this issue as once we solve the Safari issue, the thickness one will be straightforward.

@zannager zannager added the component: CircularProgress The React component. label Mar 14, 2023
@zannager zannager added the scope: progress Changes related to the progress. label Sep 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

4 participants