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

useSize fix; axis label adjustments; distribution hover improvements #1377

Merged
merged 8 commits into from
Nov 14, 2022

Conversation

berekuk
Copy link
Collaborator

@berekuk berekuk commented Nov 10, 2022

I wanted to fix #1356 but fixed a bunch of other things by accident.

First, I replaced useSize with useMeasure. useMeasure uses ResizeObserver API so it reacts to window resizes correctly.

Second, I don't render the chart at all if the width is not known (i.e., on the server side and on the first render). This allowed me to drop the width prop from the components code

Third, I wrapped the component in overflow: auto, so that it doesn't resize itself infinitely.

Turns out that was enough to fix #462 (server-side rendering), so I also removed the FallbackSpinner code and dynamic component proxies from website/components/*.

Minor consequence: the initial documentation load is slower (since squiggle-components bundle is loaded immediately), but it's tolerable, and we can always bring the dynamic loading back if we don't like it. On fast enough machines the new version is better because the text doesn't jump around after the spinner is replaced with the rendered chart.

This also completely fixes #506. And #1241 (width is now fine both when you stretch the playground and reduce it back to the previous width; previously it didn't shrink back to the original size). So, 4 3 issues combo 😁

One problem remains: sometimes the chart doesn't fit the parent component, and instead of that infinite stretch animation it now adds a scroll bar. My impression is that this is due to ticks adding an extra width. I'm still investigating how this could be fixed.

@vercel
Copy link

vercel bot commented Nov 10, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
squiggle-components ✅ Ready (Inspect) Visit Preview Nov 11, 2022 at 9:35AM (UTC)
squiggle-website ✅ Ready (Inspect) Visit Preview Nov 11, 2022 at 9:35AM (UTC)

@codecov-commenter
Copy link

Codecov Report

Merging #1377 (8a2efcf) into develop (48d69a3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #1377   +/-   ##
========================================
  Coverage    52.11%   52.11%           
========================================
  Files           19       19           
  Lines          401      401           
  Branches        23       23           
========================================
  Hits           209      209           
  Misses         190      190           
  Partials         2        2           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@berekuk
Copy link
Collaborator Author

berekuk commented Nov 10, 2022

Turns out that was enough to fix #462

I was mistaken about this, it's not enough, docusaurus start doesn't seem to be doing SSR. That's why builds are failing.

@berekuk
Copy link
Collaborator Author

berekuk commented Nov 11, 2022

I had to revert SSR improvements.

Turns out docusaurus doesn't play well with React.lazy because React.lazy relies on React 18 Suspense.

I could probably figure it out with react-loadable or something else, but would prefer to do #774 instead and replace react-ace entirely.

@berekuk
Copy link
Collaborator Author

berekuk commented Nov 11, 2022

My impression is that this is due to ticks adding an extra width. I'm still investigating how this could be fixed.

Yeah. So I added two things here:

  • enabled labelFlush option that shifts the left and right ticks to fit inside the axis
  • set tickSize=2 (previously ticks were disabled) so that tick position would be more clear

There are still pathological cases when scrollbar can appear: if the second-from-left or second-from-right tick is too long then it can overflow. Example: https://squiggle-website-git-usesize-fix-quantified-uncertainty.vercel.app/playground#code=eNqrVirOyC8PLs3NTSyqVLIqKSpN1QELuaZkluQXwURSUtMSS3NKnPNTUpWslPLyi3ITczQMdAxTdQ0NDDSVagGn8RhQ

But that output is broken in other ways so it doesn't seem that important to fix. And I still kept the width - 22 adjustment, so we have some leeway when the overflow is not that big.

@berekuk berekuk marked this pull request as ready for review November 11, 2022 09:38
@berekuk berekuk changed the title useSize fix; SSR; distribution hover improvements useSize fix; axis label adjustments; distribution hover improvements Nov 11, 2022
@berekuk
Copy link
Collaborator Author

berekuk commented Nov 11, 2022

This is now ready for review. Merging this is necessary for the upcoming 0.5.1 release.

@berekuk berekuk merged commit 9d81f0f into develop Nov 14, 2022
@berekuk berekuk deleted the usesize-fix branch November 14, 2022 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants