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

Chart rendering and localstorage logic cleanup #40

Merged
merged 7 commits into from
Jun 22, 2024
Merged

Conversation

camdendotlol
Copy link
Owner

@camdendotlol camdendotlol commented May 18, 2024

Summary

This PR fixes some inefficiencies in the code related to rendering charts and writing to localStorage.

Chart rendering

  • removes some unnecessary renderChart() calls in places where the chart is also rendered automatically due to the store.subscribe callback (should bring a minor increase in chart performance)
  • removes some chart data migration code that wasn't really necessary as the topster package already assumes the same set of defaults when the checked attributes are missing (cuts down on memory usage when a chart is loaded as we were cloning the entire chart to run the migration)
  • separates most of the logic related to writing to localStorage from the code that reads from localStorage
    • adds a new LocalStorageWatcher renderless component at the top of the DOM intended be the only component that writes/reads directly to/from localStorage
      • previously there was a mess of components going back and forth between localStorage and the Vuex state - this should make the data flow more coherent:
        1. All other components solely read from and write to the Vuex state
        2. LocalStorageWatcher watches the Vuex state and updates localStorage when it changes
        3. Aside from the very first render, when Vuex state is populated from localStorage, data flows only one way: Vuex -> localStorage. An update to localStorage should never trigger a re-render.
      • TopBar and Switcher still communicate with localStorage because they interact with the currentlyActive localStorage item. It might be worth creating a new state item to track this value so we can give it the same one-way treatment as above.
    • this should make weird and hard-to-debug render loops easier to avoid

Interaction Layer

Additionally, this PR splits the ChartCanvas component into two as suggested by #13:

  1. ChartCanvas contains the canonical rendered chart (plus empty item placeholders) and only re-renders when the chart state changes.
  2. InteractionCanvas handles the drag-and-drop interactivity. This canvas is set to the same size as the ChartCanvas and sits on top of it, but the only thing it ever renders is when the user is dragging an item around on the chart.

This system nearly eliminates the jankiness that could occur when moving around items on a large chart on a slower device, as we're not re-rendering everything on the chart for every frame of movement.

Notes

It's possible that the localStorage refactor will unblock #36.

Copy link

netlify bot commented May 18, 2024

Deploy Preview for topsters3 ready!

Name Link
🔨 Latest commit 0b877d5
🔍 Latest deploy log https://app.netlify.com/sites/topsters3/deploys/6649097b99c6d5000820688c
😎 Deploy Preview https://deploy-preview-40--topsters3.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 configuration.

@camdendotlol camdendotlol merged commit 1a3be05 into main Jun 22, 2024
1 check passed
@camdendotlol camdendotlol deleted the localstorage branch June 23, 2024 03:11
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.

1 participant