chore(explore): Save Resizable width to localStorage#12593
Conversation
Codecov Report
@@ Coverage Diff @@
## master #12593 +/- ##
==========================================
- Coverage 66.77% 66.64% -0.13%
==========================================
Files 1021 1021
Lines 49967 49979 +12
Branches 4890 5022 +132
==========================================
- Hits 33364 33311 -53
- Misses 16478 16543 +65
Partials 125 125
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Does this PR affect only Dataset side edition? |
|
@adam-stasiak I believe this specific ticket was to address only these resizable components: #12411 (comment) |
|
I see. @junlincc Do you think we should create Issue for having the middle part of view also kept in memory? @nikolagigic One thing is weird that the value in local storage can be lower than 0. You can reproduce this by draging tab far left (to the right side also but then we have very high number). Maybe it should me limited to real values? |
absolutely, that's the plan. Let's create a separate issue. @pkdotson is working on something related @adam-stasiak |
|
@adam-stasiak I will address this. thanks for noting it |
|
I created issue for this: #12599 |
|
@nikolagigic @adam-stasiak can we take a look at the failing tests? would like to include this PR in v1.0.1 |
|
@junlincc changes have nothing to do with this failing test. Reopened the PR |
| function onResize(evt) { | ||
| const { x } = evt; | ||
| if (x > 0) { | ||
| localStorage.setItem('explore_sidebar_widths', x); |
There was a problem hiding this comment.
You might want to check that localStorage is available or catch eventual errors. It might be disabled and will throw an exception in that case.
| } | ||
|
|
||
| const exploreSidebarWidth = | ||
| localStorage.getItem('explore_sidebar_widths') || 320; |
| <Resizable | ||
| defaultSize={{ width: 300 }} | ||
| onResize={onResize} | ||
| defaultSize={{ width: exploreSidebarWidth }} |
There was a problem hiding this comment.
I think the dataset pane and the controls pane should have separate widths. You might want to create a pair of small getter/setter functions to handle saving and reading both widths in one key safely.
enum StorageKey {
sidebarWidth = "explore_sidebar_widths"
}
function getSidebarWidths() {
try {
return localStorage.getItem(StorageKey.siderbarWidth).split(":");
} catch {}
return [300, 320]
}
function setSidebarWidths(widths: number[]) {
try {
localStorage.setItem(StorageKey.siderbarWidth, widths.join(":"));
} catch {}
}There was a problem hiding this comment.
I noticed SQL Lab is using localstorage without a try/catch safety net. This PR makes me wonder if we should make an abstracted helper method, kind of like the "logging" method from @superset-ui/core. These methods, maybe setStoredValue and getStoredValue, could try local storage, then try setting a cookie, and take an optional callback for the catch block. I don't think we need to get that crazy with this PR, but wonder if folks think it's an idea worth ticketing separately.
There was a problem hiding this comment.
I always do that in my other projects. Here's an example file with typing for storage keys & values:
import { User } from 'graphql/job.graphql';
/**
* Local storage manager
*/
export function safeJsonParse(jsonString: string | null) {
if (jsonString === null) return null;
try {
return JSON.parse(jsonString);
} catch {
return null;
}
}
export type StorageKey = 'persistedStates' | 'hasWelcomed';
export type StorageValues = {
persistedStates: Record<string, any> | null;
hasWelcomed: boolean;
};
export const defaultStorageValues = {
userIdMap: null,
persistedStates: null,
hasWelcomed: false,
};
export function entries() {
Object.values(Storage).forEach(key => {
return [key, getItem(key)];
});
}
export function getItem<K extends StorageKey>(key: K): StorageValues[K] | null {
return safeJsonParse(localStorage.getItem(key)) ?? defaultStorageValues[key];
}
export function setItem(key: StorageKey, value: string | number | object) {
return localStorage.setItem(key, JSON.stringify(value));
}|
As to @ktmud's point, I agree the two panes should be sizeable independently, and have their values stored independently. You could save them together as he proposes here, or with two getter/setters. Whatever seems cleaner. The bigger issue to me with the current state of the PR is what's seen in the description GIF. If you make the data source panel wide, the controls panel does not scale (this is fine). However, when you refresh, the controls panel is big, too. Resizing the controls panel (at the line to the right of it) yeilds unpredictable results as well. It seems like whether the controls panel is small, medium or large, if you widen the controls panel, and then refresh, both columns become full width. column-resize-issue.mp4 |
|
Also note folding the dataset pane should be persistent as well. You can probably save a |
|
@ktmud @zhaoyongjie guys, can you take a look at the changes? if they look ok, can we merge? thank you both~ |
ktmud
left a comment
There was a problem hiding this comment.
LGTM.
Thought of persisting the folding state, too, but then realized there is not easy access to change/edit dataset once it's folded.
|
@nikolagigic can you solve the merge conflicts? |
|
@ktmud hey we resolved the conflicts, mind taking another look? Thanks! |
SUMMARY
Save Resizable component defaultSize value to localStorage.
closes #12428
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION