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

fix: zero value for min and max ticks #78

Merged
merged 3 commits into from Feb 9, 2022

Conversation

MaherSaleem
Copy link
Contributor

Fix the case where ticks min/max is zero

Description

I supported the case for Number(v) || v when v is 0.

Motivation and Context

I was using the lwcc, and wanted to make the min value as 0, but then realized that it's not working, so I digged into the code and realized that this case is not handled

How Has This Been Tested?

Testsed on a chart

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@scolladon scolladon self-requested a review October 28, 2021 08:53
@victorgz
Copy link
Collaborator

victorgz commented Feb 8, 2022

Hi @MaherSaleem , thanks for your contribution !

I've suggested some changes, because actually the fix is much simpler than what you proposed. Both attributes are expecting a Number, so we don't need to have a default fallback value. We can just cast the value we receive into a Number.

I've reproduced the case in an org and it is working (before the fix the chart was not even rendering with ticks-min="0")

<c-chart type="horizontalBar">
	<c-dataset labels='["Eating", "Drinking", "Sleeping", "Designing", "Coding", "Cycling", "Running"]'>
		<c-data label="My dataset" detail={data} backgroundcolor="rgba(255, 176, 59, 1)"></c-data>
	</c-dataset>
	<c-title text="Radar Chart"></c-title>
	<c-cartesian-category-axis axis="y" position="left"></c-cartesian-category-axis>
	<c-cartesian-linear-axis axis="x" ticks-suggestedmax="200" ticks-min="0" ></c-cartesian-linear-axis>
</c-chart>

image

Also, could you please update the PR title to follow the conventional commits convention? Otherwise the CI checks won't pass. A suggestion for it:

fix: zero value for min and max ticks

Thanks !

@MaherSaleem MaherSaleem changed the title Fix the case where ticks min/max is zero fix: zero value for min and max ticks Feb 9, 2022
MaherSaleem and others added 2 commits February 9, 2022 10:13
Co-authored-by: Victor Garcia Zarco <victor.gzarco@gmail.com>
Co-authored-by: Victor Garcia Zarco <victor.gzarco@gmail.com>
@MaherSaleem
Copy link
Contributor Author

Thanks @victorgz for your change, it's really simpler! I've changed the PR name as well.

Now there is a CI / scratch-org-test (pull_request_target) , I'm not sure why

@victorgz
Copy link
Collaborator

victorgz commented Feb 9, 2022

Thanks @victorgz for your change, it's really simpler! I've changed the PR name as well.

Now there is a CI / scratch-org-test (pull_request_target) , I'm not sure why

You don't have to worry about that. We had some problems to run CI actions when the PRs where coming from a fork (see #95 ).

@scolladon is this PR ok for you ?

Copy link
Collaborator

@scolladon scolladon left a comment

Choose a reason for hiding this comment

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

LGTM !

Nice catch and lean solution !

@victorgz victorgz merged commit 4cbfe80 into SalesforceLabs:master Feb 9, 2022
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.

None yet

3 participants