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: no default axis hooks bug #424

Merged
merged 8 commits into from
Jun 28, 2022

Conversation

wadjih-bencheikh18
Copy link
Contributor

closes : #423

@netlify
Copy link

netlify bot commented Jun 24, 2022

Deploy Preview for react-plot ready!

Name Link
🔨 Latest commit a31c862
🔍 Latest deploy log https://app.netlify.com/sites/react-plot/deploys/62bae2aec439bd0008e85adc
😎 Deploy Preview https://deploy-preview-424--react-plot.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@targos
Copy link
Member

targos commented Jun 24, 2022

Do we already have a story with this case? If not, can you add one?

@wadjih-bencheikh18
Copy link
Contributor Author

Do we already have a story with this case? If not, can you add one?

the problem is in zakodium-oss/react-science#166

https://deploy-preview-166--analysis-ui-components.netlify.app/storybook/?path=/story/layout-measurementexplorer--control choose xVariable : "t"

@wadjih-bencheikh18
Copy link
Contributor Author

i will try to create a story with the problem

@wadjih-bencheikh18
Copy link
Contributor Author

@targos story added

@targos
Copy link
Member

targos commented Jun 26, 2022

How can I reproduce the problem with the story? I tried to add a log when the value is undefined but nothing happens.

@targos
Copy link
Member

targos commented Jun 26, 2022

I think it would be better to show the bug with a very simple story instead of this one.

@wadjih-bencheikh18
Copy link
Contributor Author

I think it would be better to show the bug with a very simple story instead of this one.

the problem happend while changing the data after the plot loaded so i added a story where u can change horizantal and vertical variables

@wadjih-bencheikh18
Copy link
Contributor Author

the bug always happend if we revert this modification 2aac582

@targos
Copy link
Member

targos commented Jun 27, 2022

But for me this is not the correct fix. The crosshair doesn't work as expected.

@wadjih-bencheikh18
Copy link
Contributor Author

But for me this is not the correct fix. The crosshair doesn't work as expected.

Sorry, i didn't notice that,I will check

@wadjih-bencheikh18
Copy link
Contributor Author

wadjih-bencheikh18 commented Jun 27, 2022

@targos i found the problem and fix it
the thing that hooks were not genralized to use with no default axis variables

  • i added DualAxisOptions in useAxisZoom to fix the problem
  • convertStrings changes reverted (Logically, the case wouldn't happen)

@wadjih-bencheikh18 wadjih-bencheikh18 changed the title fix: convertString bug fix: no default axis hooks bug Jun 27, 2022
@targos
Copy link
Member

targos commented Jun 28, 2022

Vertical zoom doesn't work (it's horizontal), but apart from that, good job!
https://deploy-preview-424--react-plot.netlify.app/?path=/story/examples-measurement--measurement&args=xAxis:a;yAxis:t;hook:Vertical+Zoom

@wadjih-bencheikh18
Copy link
Contributor Author

Vertical zoom doesn't work (it's horizontal)

Sorry about that , it's fixed now

@targos targos merged commit 0aa6f36 into zakodium-oss:main Jun 28, 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.

Bug
2 participants