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

refactor: store accordion preference #815

Merged
merged 30 commits into from
Nov 18, 2024

Conversation

patricio0312rev
Copy link
Contributor

@patricio0312rev patricio0312rev commented Nov 13, 2024

[portfolio] store table state

Summary

  • Depends on refactor: set accordions opened by default #813
  • use-accordion hook has been refactored to store a key for each implementation in local storage. This refactor will allow to store the preference of the user regarding the accordion being opened or not on reload.
  • The unit tests have been refactored.

Checklist

  • My changes look good in both light AND dark mode
  • The change is not hardcoded to a single network, but has multi-asset in mind
  • I checked my changes for obvious issues, debug statements and commented code
  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

Copy link

vercel bot commented Nov 13, 2024

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

Name Status Preview Updated (UTC)
arkvault ✅ Ready (Inspect) Visit Preview Nov 15, 2024 5:06pm

Copy link

@samharperpittam samharperpittam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patricio0312rev - is it possible to make this behaviour function independently of different profiles?

Currently, if you have 2 or more profiles setup, the accordian state is applied to all profiles. For example

  • If I have 2 profiles (ProfileA and ProfileB)
  • I open profile A
  • I set Mainnet accordian to open. Devnet to closed
  • This state should just be saved to profile A
  • When I open profile B, it should open with the default view.

Currently whatever changes you make to profileA get applied to profileB too.

@samharperpittam samharperpittam marked this pull request as draft November 14, 2024 15:27
@patricio0312rev patricio0312rev marked this pull request as ready for review November 14, 2024 18:01
@patricio0312rev
Copy link
Contributor Author

@samharperpittam should be fixed now 🌟

const storageKey = getStorageKey(key);
const [isExpanded, setIsExpanded] = useState<boolean>(() => {
const storedValue = localStorage.getItem(storageKey);
return storedValue ? JSON.parse(storedValue) : true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove the JSON.parse part, because it stores only bool primitives which don't require deserialization, unless your plan was keeping data in a single key.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I am parsing to keep the consistency of the booleans. If I remove this, we could get values like "true", "false" and that could lead to false positives.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case storing 1 or 0 could resolve the issue 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it simplifies the issue with "true" and "false", that would reduce readability, and seeing that there are a lot of components and hooks in the repo, I wouldn't take that path 🤔

@ItsANameToo ItsANameToo merged commit 29273b3 into develop Nov 18, 2024
20 checks passed
@ItsANameToo ItsANameToo deleted the refactor/store-accordion-preference branch November 18, 2024 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants