-
Notifications
You must be signed in to change notification settings - Fork 12
Conversation
Deploy preview for catwalk-qlikcore ready! Built with commit 64b7b0c |
src/components/cube.jsx
Outdated
@@ -28,6 +32,41 @@ const Cube = forwardRef(({ app, tableData: { initialColumns }, closeOnClickOutsi | |||
let model = null; | |||
let hypercubeProps = null; | |||
|
|||
const modifyLocalStorage = (action) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps use something like https://github.com/rehooks/local-storage ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to constantly read/write to localstorage (to keep sync with some kind of state). Otherwise it's just replacing one line with another line and also adding a new dependency.
src/components/cubes.jsx
Outdated
@@ -42,6 +48,22 @@ export function Cubes({ app, closeOnClickOutside }) { | |||
forceUpdate(); | |||
} | |||
|
|||
useEffect(() => { | |||
// Check if cubes are stored in localstorage. | |||
let storedCubes = localStorage.getItem(app.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this can throw an error if local storage is disabled, we probably need to gracefully disable this feature if we cannot use LS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I added a check if localstorage is disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes #237