Skip to content

chore(explore): Save Resizable width to localStorage #12593

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.

Sign up for GitHub

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

Merged
ktmud merged 4 commits into from
Jan 25, 2021
+43 −4
Merged

chore(explore): Save Resizable width to localStorage #12593

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2bba6c0
Save resizable width to localStorage
nikolagigic Jan 19, 2021
6eefc0f
Adress comments
nikolagigic Jan 19, 2021
e4bb369
adress comments
nikolagigic Jan 21, 2021
4b8177b
Merge branch 'master' into issue-12428
nikolagigic Jan 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions superset-frontend/src/explore/components/ExploreViewContainer.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,10 +365,20 @@ function ExploreViewContainer(props) {
);
}

function onResize(evt) {
const { x } = evt;
if (x > 0) {
localStorage.setItem('explore_sidebar_widths', x);

@geido geido Jan 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

}
}

if (props.standalone) {
return renderChartContainer();
}

const exploreSidebarWidth =
localStorage.getItem('explore_sidebar_widths') || 320;

@geido geido Jan 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same as my other comment


return (
<Styles id="explore-container" height={height}>
<Global
Expand Down Expand Up @@ -404,7 +414,8 @@ function ExploreViewContainer(props) {
/>
)}
<Resizable
defaultSize={{ width: 300 }}
onResize={onResize}
defaultSize={{ width: exploreSidebarWidth }}

@ktmud ktmud Jan 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 {}
}

@rusackas rusackas Jan 20, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@ktmud ktmud Jan 20, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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));
}

minWidth={300}
maxWidth="33%"
enable={{ right: true }}
Expand Down Expand Up @@ -456,7 +467,8 @@ function ExploreViewContainer(props) {
</div>
) : null}
<Resizable
defaultSize={{ width: 320 }}
onResize={onResize}
defaultSize={{ width: exploreSidebarWidth }}
minWidth={320}
maxWidth="33%"
enable={{ right: true }}
Expand Down