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

localStorage Features #11

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

BerndSchmecka
Copy link

This PR Features:

  • Updated dependencies
  • Added saving image to localStorage
  • Added saving options (not room structure yet) to localStorage

PS: I'm sorry if my code is not 100% efficient, I'm more a Vue.js than a React guy ;)

@pitkes22
Copy link
Owner

Hi, Thank you for making the PR 👍 I will try to review it later this week.

@pitkes22
Copy link
Owner

pitkes22 commented Nov 3, 2022

Hi, I have looked through it and it looks good. 👍

The only issue is that there are some visual problems with inputs and other elements. It looks like blueprint JS has changed from using blue-ish colors to gray-ish colors and now it does not look that great with TeamSpeak blue background.

image

@BerndSchmecka
Copy link
Author

Thank you for looking through my PR!

I added a new commit to improve the looks of those elements. I realized that I forgot to update bp3-dark to bp4-dark after upgrading the dependencies. 😅 Now I did that and also used styled-components to override the gray box-shadow which is used in blueprint 4. So now it should look better.

@pitkes22
Copy link
Owner

pitkes22 commented Nov 6, 2022

That looks much better 👍
I have played with it a little and I have found one more bug that will have to be fixed.

Steps to reproduce

  1. Clear localStorage
  2. Upload the Image
  3. Check LS (savedImage is set, but no options are set)
  4. Reload the Page
  5. Options are set to invalid values (only single channel, all values are 0, etc...)
  6. To actually set options you have to manually edit the options

I think the solution is to simplify a bit how options are serialized and saved to localStorage. Instead of using useEffect and checking optionUpdateAllowed during rendering cycle, you can implement useLocalStorage hook that will provide a similar interface as the useState hook but it will save its state to localStorage. You can find many implementations on the internet, so you can add it in the new file, or if you found a good library add it as a dependency. The only caveat of this is that we are using NextJs, so the page is first server-side rendered. During SSR there is no localStorage available, so the default value will be used and then when the page is hydrated on the client-side it will not match server HTML since it will render differently due to values from localStorage. So you will have to find the implementation of useLocalStorage that can handle that.

@BerndSchmecka
Copy link
Author

Thank you for pointing out that bug 👍

I did what you said (I found use-local-storage-state, which explicitly supports SSR and works great for this purpose). Now everything (at least according to my testing) should work correctly.

@pitkes22
Copy link
Owner

pitkes22 commented Nov 8, 2022

Looks much better 👍 , one more question. Wouldn't it be possible to get rid of the optionUpdateAllowed? Because it is kinda anti-pattern in react. What would happen if you used useLocalStorage also for image state? Since localStorage is synchronous I think it should load the image from localStorage during the first render so there will be no intermediate state where the page is rendered but the image was not yet loaded.

@BerndSchmecka
Copy link
Author

I did try that, but unfortunately it still resets the options for some reason

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.

2 participants