Skip to content
This repository has been archived by the owner on Nov 10, 2017. It is now read-only.

save height of down panel in local storage #62 #74

Merged
merged 4 commits into from
Apr 5, 2017
Merged

save height of down panel in local storage #62 #74

merged 4 commits into from
Apr 5, 2017

Conversation

wcastand
Copy link
Contributor

@wcastand wcastand commented Apr 4, 2017

Hi, i don't know why there are changes in all those files i only changes src/modules/ui/components/layout/index.js and src/modules/ui/components/shortcuts_help.js the last one had a eslint error.

If that's not suppose to happen (all the change in files) let me know and i'll make an other PR :)

Pr associated with the issue #62

@wcastand wcastand changed the title save heigt of down panel in local storage save height of down panel in local storage Apr 4, 2017
@wcastand wcastand changed the title save height of down panel in local storage save height of down panel in local storage #62 Apr 4, 2017
Copy link
Contributor

@benediktvaldez benediktvaldez left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for the contribution @wcastand, I've been hoping to look into this as well for a while.

Just check out my comment about making sure localStorage is usable, otherwise it looks good.

You can safely ignore all those changes to dist, it's a recurring problem we're solving.

@@ -86,6 +86,13 @@ class Layout extends React.Component {
downPanelDefaultSize = downPanelInRight ? 400 : 200;
}

if(typeof localStorage !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not enough to make sure localStorage is usable. When using Safari in private mode, you'll run into issues because localStorage is defined, but the quota is set to 0, meaning that if you try to setItem, the browser will throw an error.

Pretty simple to get around though, could do something like this:

const hasLocalStorage = () => {
  try {
      localStorage.setItem('yo', 'yo')
      localStorage.removeItem('yo')
      return true
  } catch(e) {
      return false
  }
}

@@ -108,6 +115,13 @@ class Layout extends React.Component {
resizerChildren={downPanelInRight ? vsplit : hsplit}
onDragStarted={onDragStart}
onDragFinished={onDragEnd}
onChange={
Copy link
Contributor

@benediktvaldez benediktvaldez Apr 4, 2017

Choose a reason for hiding this comment

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

Oh and also this, @wcastand check out the conflict that came up due to another PR getting merged, make sure to it plays along :)

@wcastand
Copy link
Contributor Author

wcastand commented Apr 5, 2017

The project wasn't build and the dist files aren't updated but i didn't find the npm script to build.

Copy link
Contributor

@benediktvaldez benediktvaldez left a comment

Choose a reason for hiding this comment

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

Looks great @wcastand!

@benediktvaldez benediktvaldez merged commit e01f169 into storybook-eol:master Apr 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants