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

Circular Progress: Fix the minor indeterminate animation #3681

Merged
merged 3 commits into from Jan 10, 2022

Conversation

mckaragoz
Copy link
Member

@mckaragoz mckaragoz commented Jan 5, 2022

Description

Fixes #1831, Fixes #2488 (duplicate)

As mentioned in the title.

20220105_171927.mp4

How Has This Been Tested?

Checklist:

✔️ The PR is submitted to the correct branch (dev).
✔️ My code follows the code style of this project.
❌ I've added relevant tests. (No need)

@mudbot mudbot bot added bug Something does not work as intended/expected enhancement New feature or request needs review labels Jan 5, 2022
@codecov
Copy link

codecov bot commented Jan 5, 2022

Codecov Report

Merging #3681 (0d04573) into dev (d70a2e4) will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #3681      +/-   ##
==========================================
+ Coverage   90.44%   90.49%   +0.05%     
==========================================
  Files         333      332       -1     
  Lines       10546    10491      -55     
==========================================
- Hits         9538     9494      -44     
+ Misses       1008      997      -11     
Impacted Files Coverage Δ
...azor/Components/Progress/MudProgressCircular.razor 100.00% <ø> (ø)
...r/Components/Progress/MudProgressCircular.razor.cs 92.00% <ø> (-0.86%) ⬇️
...nts/PageContentNavigation/MudPageContentSection.cs 84.21% <0.00%> (-15.79%) ⬇️
...ontentNavigation/MudPageContentNavigation.razor.cs 95.65% <0.00%> (-1.65%) ⬇️
...geContentNavigation/MudPageContentNavigation.razor 100.00% <0.00%> (ø)
...es/BindingConverters/NumericBoundariesConverter.cs
...r/Components/NumericField/MudNumericField.razor.cs 97.77% <0.00%> (+0.42%) ⬆️
...Components/ThemeProvider/MudThemeProvider.razor.cs 98.93% <0.00%> (+0.70%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d70a2e4...0d04573. Read the comment docs.

@rogue6800
Copy link
Contributor

Brill, this has been bugging me for ages!

@mikes-gh
Copy link
Contributor

mikes-gh commented Jan 7, 2022

This PR is suffering from trying to address 2 issues in 1 PR. Why don't we remove the parameter in this PR. The tests will pass and we can just merge. Then if @Garderoben thinks the rounded parameter is worth pursuing it can be done in a separate PR with a test. Another idea (for the seperate PR) is just have it rounded for indeterminate and not for other (no extra API required)

@mckaragoz
Copy link
Member Author

This PR is suffering from trying to address 2 issues in 1 PR. Why don't we remove the parameter in this PR. The tests will pass and we can just merge. Then if @Garderoben thinks the rounded parameter is worth pursuing it can be done in a separate PR with a test. Another idea (for the seperate PR) is just have it rounded for indeterminate and not for other (no extra API required)

I think testing is easy when we decide, we can simply search for rounded class.

@Garderoben
Copy link
Member

After some thought i don't think rounded makes sense at all for this component. Its meant to show progress and not really be a dumb spinner.

Remove the rounded stuff and just do the fix please.

@mikes-gh mikes-gh added needs changes and removed bug Something does not work as intended/expected needs review labels Jan 9, 2022
@mckaragoz
Copy link
Member Author

After some thought i don't think rounded makes sense at all for this component. Its meant to show progress and not really be a dumb spinner.

Remove the rounded stuff and just do the fix please.

I added this because linear progress also have rounded parameter, and wanted to make them similar for consistent library. But i am reverting the rounded parameter now. If we want it later, we know its a one line CSS.

Ekran görüntüsü 2022-01-09 123450

@mckaragoz mckaragoz changed the title Circular Progress: Fix the minor indeterminate animation and add rounded parameter Circular Progress: Fix the minor indeterminate animation Jan 9, 2022
@Garderoben
Copy link
Member

After some thought i don't think rounded makes sense at all for this component. Its meant to show progress and not really be a dumb spinner.
Remove the rounded stuff and just do the fix please.

I added this because linear progress also have rounded parameter, and wanted to make them similar for consistent library. But i am reverting the rounded parameter now. If we want it later, we know its a one line CSS.

Ekran görüntüsü 2022-01-09 123450

Yes but they are quite different, linear goes from point A to point B where the circular connects to itself.

Copy link
Member

@JonBunator JonBunator left a comment

Choose a reason for hiding this comment

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

Thank you! Great that this is finally fixed :)

@JonBunator JonBunator merged commit 25d086e into MudBlazor:dev Jan 10, 2022
@JonBunator JonBunator added bug Something does not work as intended/expected and removed enhancement New feature or request needs review labels Jan 10, 2022
@JonBunator JonBunator added this to the 6.0.5 milestone Jan 10, 2022
@mckaragoz mckaragoz deleted the CircularFix branch January 14, 2022 13:32
jammerware pushed a commit to jammerware/MudBlazor that referenced this pull request Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as intended/expected
Projects
None yet
5 participants