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

Radial bar components shifts the filled part relative to the unfilled background #4264

Closed
1 task done
AigiulGasymova opened this issue Mar 5, 2024 · 15 comments
Closed
1 task done
Labels
bug General bug label

Comments

@AigiulGasymova
Copy link

AigiulGasymova commented Mar 5, 2024

  • I have searched the issues of this repository and believe that this is not a duplicate.

Reproduction link

Edit on CodeSandbox

Steps to reproduce

Created a semicircle chart that fills with color clockwise depending on the current value and has a background, used RadialBarChart with PolarAngleAxis and RadialBar.

What is expected?

There is no offset in RadialBar like it was in elder versions - for example, 1.8.2.

What is actually happening?

When there is a background in RadialBar, the filled part has an outline from the background, and the unfilled part has an outline from the fill color, i.e. the filled part seems to be slightly offset relative to the base with the unfilled part.

Environment Info
Recharts v2.12.2
React 17.0.2
System Windows 11 Home edition
Browser Chrome 122.0.6261.95
@PavelVanecek
Copy link
Collaborator

If you allow I made screenshots of what it looks like in recharts: 1.8.2:
image

And this is what it looks like in recharts: 2.12.2:
image

@PavelVanecek
Copy link
Collaborator

It appears this is a change from 2.9 release (the one where I made my first contribution 😬 )

@AigiulGasymova
Copy link
Author

@PavelVanecek thanks for the screenshots! Yes, it appears that the issue started reproducing somewhere in the latest versions. Do we have some fixes or workarounds for this?

@PavelVanecek
Copy link
Collaborator

Looks to me like something that we should fix directly in recharts. Not sure what causes it yet. It looks to me like the problem is in the first X coordinate of path.d. Here in my codesandbox it is 65.3 in the foreground RadialBar but 64.5 in the background RadialBar (look for HTML attribute that goes d="M 64.5...":

image

If I edit it manually to be the same 64.5 then it renders fine:

image

Although when I open the same example in 2.8.0 (the working version) it appears that the 65.3000000001 was the correct one:

image

I still don't know which change in 2.9.0 caused this but hopefully this will help narrow it down

@PavelVanecek
Copy link
Collaborator

I have simplified the codesandbox a little bit, it no longer looks like a chart but I think it still demonstrates the problem: https://codesandbox.io/p/sandbox/recharts-issue-template-forked-94jylk

@AigiulGasymova
Copy link
Author

@PavelVanecek our developer had the idea that it's probably sub-pixel misalignment due to decimal numbers. We're using
cx="47%" and cy="45%" on the radial graphs, with a width and height of 340 and 280. The misalignment is happening here because 47% of 340 is a decimal number. If we change 340 to a number that has a whole number as 47% of it, then the issue goes away. This number worked for us is 340.4255319149

image
image

It seems the library is placing the bar on top using rounded numbers, but it's placing the background using decimals, hence the misalignment, or vice versa. Maybe this little investigation will help to find the reason and fix it?

@PavelVanecek
Copy link
Collaborator

Thank you for the details!

@PavelVanecek
Copy link
Collaborator

I can reproduce in this unit test:


  test('renders background in the exact same position as foreground', () => {
    // Reproduction of https://github.com/recharts/recharts/issues/4264
    const preparedData = [{ value: 42, fill: '#241084' }];
    const emptyBackgroundColor = '#D8BDF3';

    const { container, debug } = render(
      <RadialBarChart data={preparedData} height={280} width={340} cx="47%" startAngle={180} endAngle={0}>
        <RadialBar
          background={{
            fill: emptyBackgroundColor,
          }}
          dataKey="value"
          isAnimationActive={false}
        />
      </RadialBarChart>,
    );
    debug();
    const background = container.querySelector('.recharts-radial-bar-background-sector');
    expect(background).toBeInTheDocument();
    const foreground = container.querySelector('.recharts-radial-bar-sector');
    expect(foreground).toBeInTheDocument();
    expect(foreground.getAttribute('d')).toEqual(background.getAttribute('d'));
  });

@ckifer
Copy link
Member

ckifer commented Mar 5, 2024

Thanks for helping here @PavelVanecek!

So maybe we started rounding at some point? I think I remember something like that..

@ckifer
Copy link
Member

ckifer commented Mar 5, 2024

The calculation itself - this logic hasn't changed n 9 years.

Path calculation for sector - also hasn't changed.

Most likely this PR https://github.com/recharts/recharts/pull/3803/files#diff-445315e1448c74882c065c4b50377b6231aadafa6b72dd3632b233e404c71e45R280 and specifically where we start parsing things as ints

@ckifer ckifer added the bug General bug label label Mar 5, 2024
@AigiulGasymova
Copy link
Author

@ckifer sorry to bother you, but is there any update on this issue? This issue is not urgent and does not have a significant impact on anything other than appearance, and we are considering using the fractional width workaround, but if a fix is coming soon, we'd rather wait for it.

@ckifer
Copy link
Member

ckifer commented Mar 11, 2024

Hey @AigiulGasymova

No update really, the last I looked for this was my last comment above.

I can't promise anything with when, it really depends on how much free time I have which is pretty random.

@AigiulGasymova
Copy link
Author

Hey @AigiulGasymova

No update really, the last I looked for this was my last comment above.

I can't promise anything with when, it really depends on how much free time I have which is pretty random.

Sure, thanks for the answer (and for pretty cool and useful library!). We'll use the workaround and keep tracking this thread just in case!

@ckifer
Copy link
Member

ckifer commented Mar 15, 2024

PR #4295

@ckifer
Copy link
Member

ckifer commented Mar 15, 2024

released in 2.12.3

@ckifer ckifer closed this as completed Mar 15, 2024
renovate bot added a commit to SAP/ui5-webcomponents-react that referenced this issue Mar 18, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [recharts](https://togithub.com/recharts/recharts) | [`2.12.2` ->
`2.12.3`](https://renovatebot.com/diffs/npm/recharts/2.12.2/2.12.3) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/recharts/2.12.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/recharts/2.12.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/recharts/2.12.2/2.12.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/recharts/2.12.2/2.12.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>recharts/recharts (recharts)</summary>

###
[`v2.12.3`](https://togithub.com/recharts/recharts/releases/tag/v2.12.3)

[Compare
Source](https://togithub.com/recharts/recharts/compare/v2.12.2...v2.12.3)

Some more small changes/fixes while working on 3.x

#### What's Changed

##### Fix

- `Legend`: fix issue where Legend was not taken into account when
scaling the chart container by
[@&#8203;zhonglin94](https://togithub.com/zhonglin94) in
[recharts/recharts#4272
closes
[recharts/recharts#4246
- `Area`: fixed a bug where className was not assigned to areaDot by
[@&#8203;108yen](https://togithub.com/108yen) in
[recharts/recharts#4294
closes
[recharts/recharts#4290
- `RadialBar`: address regression where radial bar and its background
were off from eachother because of rounding by
[@&#8203;ckifer](https://togithub.com/ckifer) in
[recharts/recharts#4295
closes
[recharts/recharts#4264
- `ErrorBar`: do not count `null` as 0 in error bar domain by
[@&#8203;rinkstiekema](https://togithub.com/rinkstiekema) in
[recharts/recharts#4297

#### New Contributors

- [@&#8203;zhonglin94](https://togithub.com/zhonglin94) made their first
contribution in
[recharts/recharts#4272

**Full Changelog**:
recharts/recharts@v2.12.2...v2.12.3

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/SAP/ui5-webcomponents-react).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNDUuMCIsInVwZGF0ZWRJblZlciI6IjM3LjI0NS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug General bug label
Projects
None yet
Development

No branches or pull requests

3 participants