[Progress] Fix wrong error on validating sum total on multiple progress #758

exoego · 2019-05-21T05:17:19Z

Description

Validation on sum of percents now is performed before rounding, as proposed by @ryamaguchi0220 in [Progress] Fix to validate the sum of the percents before rounding #759.
Add module.helper.derivePrecision that calculates small-enough precision for multiple progress.
This is handy for users since they do not need to take care about precision for most cases.
If precision option is explicitly specified, it is used.

Testcase

https://jsfiddle.net/x1ptzvs7/

Screenshot (when possible)

Multiple progress bars are shown for the data in #757

Closes

#757

… precision

lubber-de

Why don't we just use 10 as a default value for precision?

lubber-de · 2019-05-21T07:04:00Z

Alright, my suggestion of using 10 as always default precision could lead into never reaching 100% at all.. So your approach is of course better 🙂

lubber-de

LGTM

lubber-de

LGTM

lubber-de

Sorry, but there is something wrong when the precision is set to 1. Calculation seems right, but bar color/width is messed up
https://jsfiddle.net/kmbc7fog/3/

In addition, the docs say the precision default setting is 1, although it's 0 in the code (which is correct)
This is not related to your changes, i just recognized it and we should setup an additional docs-PR to fix this and also add more information there like "0 means calculated precision, 1 means 1 decimal point , 10 means 2 decimal points, 100 means 3 decimal points, and so on)

lubber-de

Using precision:10 breaks it again as before (calculates a value > 100%)
https://jsfiddle.net/kmbc7fog/4/

-> What about comparing against the sum of values instead of the sum of percentages ?

You may join forces with @ryamaguchi0220 .
The following jsfiddle contains a combo of your PR and the changes from #759, because it works then
https://jsfiddle.net/kmbc7fog/6/

exoego · 2019-05-21T09:58:22Z

Calculation seems right, but bar color/width is messed up

All green ars are due to autoSuccess: true (default).
If autoSuccess: false, bars show each own color even when 100% reached.
Width seems fine for me if autoSuccess: false.

I took changes from #759 into this PR.

…recision # Conflicts: # src/definitions/modules/progress.js

lubber-de

All green ars are due to autoSuccess: true (default).
If autoSuccess: false, bars show each own color even when 100% reached.

As autoSuccess has true by default and IMHO that feature does not make sense when working with multiple progressbars i suggest to ignore that setting:

Fomantic-UI/src/definitions/modules/progress.js

Line 592 in fe241d0

    
           if(settings.autoSuccess && !(module.is.warning() || module.is.error() || module.is.success())) {

should be changed to

    if(settings.autoSuccess && $bars.length === 1 && !(module.is.warning() || module.is.error() || module.is.success())) {

Works as expected then: https://jsfiddle.net/kmbc7fog/7/

lubber-de · 2019-05-25T10:07:52Z

@exoego In case you missed my comment for "autoSuccess" , here is a kind reminder... Would like to get this PR approved for the next release 😉

…ltiple progress bars

exoego · 2019-05-25T11:38:28Z

Oops.
Fixed in 1497b64

lubber-de

LGTM

y0hami

LGTM

ryamaguchi0220 · 2019-05-30T02:45:12Z

@exoego @lubber-de

Oops.
In some case, the progress bar is still broken.
I'm not sure but, I think we should validate sum of the raw value instead of sum of the percents.

The case

https://jsfiddle.net/La8twh3u/

Screen shots

…fied for multiple bars

exoego · 2019-05-30T04:22:50Z

I'm not sure but, I think we should validate sum of the raw value instead of sum of the percents.

True.
Done in 28a2967

lubber-de

LGTM

y0hami

LGTM

lubber-de · 2019-06-10T12:09:06Z

Docs corrected by fomantic/Fomantic-UI-Docs#134

feat(progress): Derive precision for multiple progress with total and…

0f6b2f9

… precision

auto-assign bot requested review from ColinFrick, y0hami, lubber-de and prudho May 21, 2019 05:17

exoego added type/bug Any issue which is a bug or PR which fixes a bug state/awaiting-reviews Pull requests which are waiting for reviews lang/javascript Anything involving JavaScript labels May 21, 2019

fix(progress): Fix to validate the sum of the percents before rounding

5dcc65e

ryamaguchi0220 mentioned this pull request May 21, 2019

[Progress] Fix to validate the sum of the percents before rounding #759

Closed

lubber-de reviewed May 21, 2019

View reviewed changes

feat(progress): Fix auto-derived precision looses precision

b74dda5

lubber-de previously approved these changes May 21, 2019

View reviewed changes

feat(progress): Fix wrong description

32ce051

exoego dismissed lubber-de’s stale review via 32ce051 May 21, 2019 07:14

lubber-de approved these changes May 21, 2019

View reviewed changes

lubber-de requested changes May 21, 2019

View reviewed changes

lubber-de added this to the 2.7.6 milestone May 21, 2019

fix(progress): Validate sum of pecents before rounding

fe241d0

exoego changed the title ~~[Progress] Derive auto precision for multiple progress for convenience~~ [Progress] Fix multiple progress wrong error on validating sum total May 21, 2019

exoego changed the title ~~[Progress] Fix multiple progress wrong error on validating sum total~~ [Progress] Fix wrong error on validating sum total on multiple progress May 21, 2019

Merge remote-tracking branch 'yamaguchi/dev' into multiple-progress-p…

eb3346c

…recision # Conflicts: # src/definitions/modules/progress.js

lubber-de requested changes May 21, 2019

View reviewed changes

lubber-de added the state/awaiting-docs Pull requests which need doc changes/additions label May 21, 2019

fix(progress): Ignore autoSuccess since it does not make sense for mu…

1497b64

…ltiple progress bars

lubber-de previously approved these changes May 25, 2019

View reviewed changes

y0hami previously approved these changes May 25, 2019

View reviewed changes

y0hami removed the state/awaiting-reviews Pull requests which are waiting for reviews label May 25, 2019

exoego added 2 commits May 30, 2019 13:20

fix(progress): Avoid precision issues when values and total are speci…

28a2967

…fied for multiple bars

fix(progress): Sum as numbers, instead of concatening number and string.

7f64554

exoego dismissed stale reviews from y0hami and lubber-de via 7f64554 May 30, 2019 04:21

lubber-de approved these changes May 30, 2019

View reviewed changes

lubber-de requested a review from y0hami May 30, 2019 08:30

y0hami approved these changes Jun 10, 2019

View reviewed changes

y0hami merged commit 52bfd66 into fomantic:develop Jun 10, 2019

lubber-de added state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo and removed state/awaiting-docs Pull requests which need doc changes/additions labels Jun 10, 2019

exoego deleted the auto-precision-for-multiple-progress branch July 12, 2019 23:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Progress] Fix wrong error on validating sum total on multiple progress #758

[Progress] Fix wrong error on validating sum total on multiple progress #758

exoego commented May 21, 2019 •

edited

lubber-de left a comment

lubber-de commented May 21, 2019

lubber-de left a comment

lubber-de left a comment

lubber-de left a comment

lubber-de left a comment

exoego commented May 21, 2019 •

edited

lubber-de left a comment •

edited

lubber-de commented May 25, 2019

exoego commented May 25, 2019

lubber-de left a comment

y0hami left a comment

ryamaguchi0220 commented May 30, 2019 •

edited

exoego commented May 30, 2019

lubber-de left a comment

y0hami left a comment

lubber-de commented Jun 10, 2019

[Progress] Fix wrong error on validating sum total on multiple progress #758

[Progress] Fix wrong error on validating sum total on multiple progress #758

Conversation

exoego commented May 21, 2019 • edited

Description

Testcase

Screenshot (when possible)

Closes

lubber-de left a comment

Choose a reason for hiding this comment

lubber-de commented May 21, 2019

lubber-de left a comment

Choose a reason for hiding this comment

lubber-de left a comment

Choose a reason for hiding this comment

lubber-de left a comment

Choose a reason for hiding this comment

lubber-de left a comment

Choose a reason for hiding this comment

exoego commented May 21, 2019 • edited

lubber-de left a comment • edited

Choose a reason for hiding this comment

lubber-de commented May 25, 2019

exoego commented May 25, 2019

lubber-de left a comment

Choose a reason for hiding this comment

y0hami left a comment

Choose a reason for hiding this comment

ryamaguchi0220 commented May 30, 2019 • edited

The case

Screen shots

exoego commented May 30, 2019

lubber-de left a comment

Choose a reason for hiding this comment

y0hami left a comment

Choose a reason for hiding this comment

lubber-de commented Jun 10, 2019

exoego commented May 21, 2019 •

edited

exoego commented May 21, 2019 •

edited

lubber-de left a comment •

edited

ryamaguchi0220 commented May 30, 2019 •

edited