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

logarithmic scale - support of : beginAtZero, zero values, multidataset selection (issue-#9629) #10714

Closed
wants to merge 10 commits into from

Conversation

skalimer0
Copy link

@skalimer0 skalimer0 commented Sep 27, 2022

beginAtZero support and zero values display on logarithmic scale with multi dataset selection.

same changes can be apply on 3.9 branch.

issue #9629

Thanks

@skalimer0 skalimer0 changed the title logarithmic scale - support of : beginAtZero, zero values, multidataset selection logarithmic scale - support of : beginAtZero, zero values, multidataset selection (issue-#9629) Sep 27, 2022
@LeeLenaleee
Copy link
Collaborator

You don't need to open a new issue every time you change your code

@skalimer0
Copy link
Author

You don't need to open a new issue every time you change your code

Sorry for that, i just want to make that clean.

Copy link
Collaborator

@LeeLenaleee LeeLenaleee left a comment

Choose a reason for hiding this comment

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

Some minor things, need to give it a better look later.

CI is failing atm, please make sure it passes. you can check this locally by running pnpm run test. If the linting and test pass it is good.

src/scales/scale.logarithmic.js Outdated Show resolved Hide resolved
src/scales/scale.logarithmic.js Outdated Show resolved Hide resolved
src/scales/scale.logarithmic.js Outdated Show resolved Hide resolved
src/scales/scale.logarithmic.js Outdated Show resolved Hide resolved
@kurkle
Copy link
Member

kurkle commented Sep 28, 2022

Looks like the code already had support for zero, does it not work?

@skalimer0
Copy link
Author

Looks like the code already had support for zero, does it not work?

The code don't display zero. Line chart is cut.

@kurkle
Copy link
Member

kurkle commented Sep 28, 2022

Looks like the code already had support for zero, does it not work?

The code don't display zero. Line chart is cut.

true, a pen using master: https://codepen.io/kurkle/pen/KKRQpgR

@skalimer0
Copy link
Author

And no '0' tick appear too.

@skalimer0
Copy link
Author

Just an idea...
to support zero values, It may be better to use log1n() function instead of log10().... no ?

@skalimer0
Copy link
Author

I check tests on scale-logarithmic, all tests with type bar are failed because bar have beginAtZero at true by default.
All test on min value on it return 0 with my code... it's normal (current master don't have beginAtZero full support). I should add beginAtZero to false or use line type on tests ?

@LeeLenaleee
Copy link
Collaborator

LeeLenaleee commented Sep 29, 2022

Changing the type to line is fine.

Also should add a test that beginAtZero actually works

@skalimer0
Copy link
Author

Hello,
Please this feature can be merge on master ?

@LeeLenaleee
Copy link
Collaborator

The build fails, as long as that happens this can't be merged.

Also like I said in my previous comment, a test for this should be added so we get notified in the future if it breaks

@skalimer0
Copy link
Author

For the build, sorry a mistake in my commit...

@skalimer0 skalimer0 closed this Jun 13, 2024
@skalimer0
Copy link
Author

I'll do in other way later (and not in my master branch)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants