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

feat: remember pane sizes #2556

Merged
merged 9 commits into from
Aug 9, 2022

Conversation

sagar290
Copy link

@sagar290 sagar290 commented Aug 3, 2022

Closes #2456

Description

Save the different pane sizes (for instance, in local storage) in order to restore the visual configuration the next time the app is opened

Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

Additional Information

@anwarulislam anwarulislam self-requested a review August 3, 2022 07:56
Copy link
Member

@anwarulislam anwarulislam left a comment

Choose a reason for hiding this comment

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

We're using this AppPaneLayout for many pages. And we should remember pane sizes for every page separately. Your implementation is just remembering globally. So, whenever we change pane size it affects all the pages wherever we use the PaneLayout.

Instead of doing it globally we can get the layout-id as prop from the parent component. And use that prop as a prefix to define keys. Also, don't use localStorage directly to the component. We already have the implementation to store values in localpersistence.ts file. You can use that function to store.

@sagar290 sagar290 force-pushed the feat/remember-pane-size branch 2 times, most recently from 879a01d to f6f3f38 Compare August 3, 2022 20:00
@anwarulislam anwarulislam self-requested a review August 4, 2022 13:57
Copy link
Member

@anwarulislam anwarulislam left a comment

Choose a reason for hiding this comment

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

We still have the same issue. the default value is getting applied to every page if there's no layout-id. Instead of doing that way just do not store/restore values if there's no layout-id. And go with the default values.

@anwarulislam
Copy link
Member

@AndrewBastin, it seems okay to me.

@anwarulislam anwarulislam self-requested a review August 6, 2022 00:15
Copy link
Member

@anwarulislam anwarulislam left a comment

Choose a reason for hiding this comment

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

Few more changes are required

anwarulislam
anwarulislam previously approved these changes Aug 6, 2022
Copy link
Member

@anwarulislam anwarulislam left a comment

Choose a reason for hiding this comment

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

Seems okay to me. @AndrewBastin @liyasthomas

Copy link
Member

@AndrewBastin AndrewBastin left a comment

Choose a reason for hiding this comment

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

Just minor variable naming scheme corrections.

Also, this is only implemented for documentation.vue as of right now, other pages should also implement this thing.

(note to reviewer: @anwarulislam make sure you check variable naming schemes on your reviews as well)

packages/hoppscotch-app/components/app/PaneLayout.vue Outdated Show resolved Hide resolved
packages/hoppscotch-app/components/app/PaneLayout.vue Outdated Show resolved Hide resolved
packages/hoppscotch-app/components/app/PaneLayout.vue Outdated Show resolved Hide resolved
packages/hoppscotch-app/components/app/PaneLayout.vue Outdated Show resolved Hide resolved
@anwarulislam
Copy link
Member

(note to reviewer: @anwarulislam make sure you check variable naming schemes on your reviews as well)

Noted!

Copy link
Member

@AndrewBastin AndrewBastin left a comment

Choose a reason for hiding this comment

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

lgtm 💯

@liyasthomas a final round of checks and you can merge it in.

Copy link
Member

@liyasthomas liyasthomas left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@liyasthomas liyasthomas changed the title Remember pane sizes feat: remember pane sizes Aug 9, 2022
@liyasthomas liyasthomas merged commit 2e1ca0c into hoppscotch:main Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature]: Remember pane sizes
4 participants