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.

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

Jump to bottom

feat: Save datapanel state in local storage #12996

Merged
merged 4 commits into from
Feb 9, 2021
Merged

feat: Save datapanel state in local storage #12996

merged 4 commits into from
Feb 9, 2021

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Feb 8, 2021 •
edited
Loading

SUMMARY

This PR implements persisting datapanel size and open/close state in local storage. When user opens explore page again, state from local storage will be used; if local storage is empty, default values will be used (data panel closed, split sizes 90%/10%).

CC: @junlincc @villebro

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Nagranie.z.ekranu.2021-02-8.o.11.11.15.mov

TEST PLAN

ADDITIONAL INFORMATION

@codecov-io
Copy link

codecov-io commented Feb 8, 2021 •
edited
Loading

Codecov Report

Merging #12996 (575bc7c) into master (b56aec7) will increase coverage by 12.20%.
The diff coverage is 33.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #12996       +/-   ##
===========================================
+ Coverage   49.59%   61.79%   +12.20%     
===========================================
  Files         480      537       +57     
  Lines       17315    20128     +2813     
  Branches     4485     5258      +773     
===========================================
+ Hits         8587    12439     +3852     
+ Misses       8728     7475     -1253     
- Partials        0      214      +214
Flag Coverage Δ
cypress ?
javascript 61.79% <33.33%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ntend/src/explore/components/ExploreChartPanel.jsx 16.17% <25.00%> (-65.08%) ⬇️
superset-frontend/src/explore/exploreUtils.js 59.41% <28.57%> (+1.74%) ⬆️
...frontend/src/explore/components/DataTablesPane.tsx 24.05% <42.85%> (-29.08%) ⬇️
superset-frontend/src/views/App.tsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/views/menu.tsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/views/index.tsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/SqlLab/index.tsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
... and 454 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b56aec7...575bc7c. Read the comment docs.

@junlincc junlincc added explore:datapanel Related to the Data panel of Explore and removed Polidea labels Feb 8, 2021
@junlincc
Copy link
Member

junlincc commented Feb 8, 2021

welcome back @kgabryje!!!thanks for opening the PR! we need to get you back on Slack too! 🎉
This is a good step! not sure if we should store table tab and page in local storage as well? @vylc wdyt?

@nikolagigic @pkdotson please help review!

Screen.Recording.2021-02-08.at.7.56.01.AM.mov

nikolagigic
nikolagigic approved these changes Feb 8, 2021
View reviewed changes
Copy link
Contributor

@nikolagigic nikolagigic left a comment

Choose a reason for hiding this comment

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

LGTM

vylc
vylc approved these changes Feb 8, 2021
View reviewed changes
Copy link

@vylc vylc left a comment

Choose a reason for hiding this comment

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

looks good!

@junlincc junlincc added hold! On hold hold:testing! On hold for testing and removed hold! On hold labels Feb 8, 2021
pkdotson
pkdotson approved these changes Feb 8, 2021
View reviewed changes
Copy link
Member

@pkdotson pkdotson left a comment

Choose a reason for hiding this comment

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

LGTM!

@junlincc junlincc self-requested a review February 9, 2021 00:36
rusackas
rusackas approved these changes Feb 9, 2021
View reviewed changes
Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

LGTM!

@rusackas rusackas merged commit 2ce7982 into apache:master Feb 9, 2021
junlincc
junlincc approved these changes Feb 9, 2021
View reviewed changes
Copy link
Member

@junlincc junlincc left a comment

Choose a reason for hiding this comment

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

approve as product sign-off ✅ Thanks for the PR! ♥️
we may wanna make tab and pagination sticky in the future, but may affect performance.

@rusackas
Copy link
Member

rusackas commented Feb 9, 2021

@nikolagigic @ktmud I like that there's now a getter/setter for LocalStorage available to anything. We could use this in lots of other places (like the expanding/contracting of Explore sidebars, other collapsible things, etc). We could also extend that helper/util to fall back to Cookies if LocalStorage is not available. If there's any contention around LocalStorage or Cookies, this would also be a central place to set feature flags.

amitmiran137 pushed a commit to simcha90/incubator-superset that referenced this pull request Feb 10, 2021
Merge branch 'master' into decouple_filter_config_params
7891786 
* master:
  fix: UI toast typo (apache#13026)
  feat(db engines): add support for Opendistro Elasticsearch (AWS ES) (apache#12602)
  fix(build): black failing on master, add to required checks (apache#13039)
  fix: time filter db migration optimization (apache#13015)
  fix: untranslated text content of Dashboard page (apache#13024)
  fix(ci): remove signature requirements for commits to master (apache#13034)
  fix: add alerts and report to default config (apache#12999)
  docs(changelog): add entries for 1.0.1 (apache#12981)
  ci: skip cypress if no code changes (apache#12982)
  chore: add cypress required checks for branch protection (apache#12970)
  Refresh dashboard list after bulk delete (apache#12945)
  Updates storybook to version 6.1.17 (apache#13014)
  feat: Save datapanel state in local storage (apache#12996)
  fix: added text and changed margins (apache#12923)
  chore: Swap Slack Url 2 more places (apache#13004)
@junlincc junlincc removed the hold:testing! On hold for testing label Mar 15, 2021
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@junlincc junlincc junlincc approved these changes

@vylc vylc vylc approved these changes

@nikolagigic nikolagigic nikolagigic approved these changes

@pkdotson pkdotson pkdotson approved these changes

@rusackas rusackas rusackas approved these changes

@villebro villebro Awaiting requested review from villebro

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels explore:datapanel Related to the Data panel of Explore size/M 🚢 1.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@kgabryje @codecov-io @junlincc @rusackas @pkdotson @nikolagigic @vylc @mistercrunch