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 unable to add markers when autoSize chart option is enabled #1281

Merged
merged 7 commits into from
Mar 15, 2023

Conversation

victorbrambati
Copy link
Contributor

@victorbrambati victorbrambati commented Feb 22, 2023

Type of PR: bugfix

PR checklist:

Overview of change:

  • The function timeScale.isEmpty() was returning true. This because there a condition with a width === 0. To resolve this issue, an initial size was added so that the markers could be recalculated.
  • I changed the condition of _recalculateMarkers() to be easier to read.

Glitch Code Sandbox link:
https://glitch.com/edit/#!/hurricane-mahogany-output

@SlicedSilver
Copy link
Contributor

@victorbrambati Thank you for your PR.
Having a quick look at the graphics-tests it appears that this additional fixes another bug I've noticed regarding the initial render not having the correct right offset (the last few bars aren't visible).

We will conduct the code review and QA process before merging, but this will hopefully be done soon.

@victorbrambati
Copy link
Contributor Author

@SlicedSilver Surprising! Any changes that you need, you can reply me.

@SlicedSilver SlicedSilver linked an issue Feb 23, 2023 that may be closed by this pull request
@SlicedSilver
Copy link
Contributor

@victorbrambati I also think it would be good to have a graphics e2e test case which covers this bug.
Is this something that you would like to work on? or would you prefer me to add the test to your PR?

I'm happy to do it but wanted to check first if you wanted to work on this.

@victorbrambati
Copy link
Contributor Author

@SlicedSilver Yes I want work on it.

@SlicedSilver
Copy link
Contributor

@SlicedSilver Yes I want work on it.

Great :) Thank you.

Here is a some documentation on the graphics tests: https://github.com/tradingview/lightweight-charts/tree/master/tests#graphics

You essentially just need to make a .js file inside this folder: tests/e2e/graphics/test-cases

There are some examples there which you can use for inspiration, and you could do something similar to the Glitch example on the bug report.

@SlicedSilver
Copy link
Contributor

Thank you @victorbrambati
The tests look good. You can clearly see that it failed on the current release (v4.0.0) and is fixed on your branch.

Golden:
1 golden

Test:
2 test

SlicedSilver
SlicedSilver previously approved these changes Feb 24, 2023
@victorbrambati
Copy link
Contributor Author

@SlicedSilver You're welcome.
This test too work to issue #1282 because the priceline is different.

src/model/series.ts Outdated Show resolved Hide resolved
src/gui/chart-widget.ts Show resolved Hide resolved
Co-authored-by: Kirill Chetverikov <kirchet@yandex.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants