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: make tabs sticky in homepage #14695

Merged
merged 7 commits into from
May 21, 2021
Merged

Conversation

pkdotson
Copy link
Member

SUMMARY

Before when user would click on tabs in homepage and navigate away it would default back to original tab setting. This pr makes the tabs sticky using local storage to get users last tab click.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-05-18.at.11.37.22.AM.mov

TEST PLAN

click tabs in welcome submenus and reload page to ensure they "stick"

ADDITIONAL INFORMATION

  • [x ] Has associated issue: [homescreen] Make all Home Page tabs "sticky" #14670
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

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.

LGTM locally. Thanks for the enhancement

@codecov
Copy link

codecov bot commented May 19, 2021

Codecov Report

Merging #14695 (c3b12ed) into master (eb9dafc) will decrease coverage by 0.01%.
The diff coverage is 67.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14695      +/-   ##
==========================================
- Coverage   77.55%   77.53%   -0.02%     
==========================================
  Files         958      959       +1     
  Lines       48549    48695     +146     
  Branches     5702     5747      +45     
==========================================
+ Hits        37651    37758     +107     
- Misses      10697    10736      +39     
  Partials      201      201              
Flag Coverage Δ
javascript 72.52% <67.44%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...-frontend/src/views/CRUD/welcome/ActivityTable.tsx 79.59% <41.66%> (-5.47%) ⬇️
...perset-frontend/src/views/CRUD/welcome/Welcome.tsx 83.87% <75.00%> (-0.58%) ⬇️
...frontend/src/views/CRUD/welcome/DashboardTable.tsx 65.78% <76.92%> (+2.15%) ⬆️
...set-frontend/src/views/CRUD/welcome/ChartTable.tsx 77.96% <78.57%> (+0.88%) ⬆️
...t-frontend/src/dashboard/components/SliceAdder.jsx 77.33% <0.00%> (-4.76%) ⬇️
...rontend/src/components/ListView/Filters/Search.tsx 82.35% <0.00%> (-4.32%) ⬇️
...erset-frontend/src/SqlLab/components/ResultSet.tsx 67.44% <0.00%> (-2.56%) ⬇️
...nd/src/explore/components/ExploreActionButtons.tsx 71.73% <0.00%> (-0.61%) ⬇️
.../src/explore/components/ControlPanelsContainer.tsx 80.85% <0.00%> (-0.58%) ⬇️
...ntend/src/explore/components/ExploreChartPanel.jsx 16.66% <0.00%> (-0.48%) ⬇️
... and 34 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 eb9dafc...c3b12ed. Read the comment docs.

@github-actions
Copy link
Contributor

@junlincc Container image not yet published for this PR. Please try again when build is complete.

@github-actions
Copy link
Contributor

@junlincc Ephemeral environment creation failed. Please check the Actions logs for details.

@rusackas rusackas requested a review from suddjian May 19, 2021 07:18
@rusackas
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@rusackas Ephemeral environment spinning up at http://35.167.45.117:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@junlincc junlincc requested a review from zhaoyongjie May 20, 2021 06:46
@rusackas
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@rusackas Ephemeral environment spinning up at http://34.220.243.232:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

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 f96fea1 into apache:master May 21, 2021
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@rusackas
Copy link
Member

Might be a good idea to follow this up with an E2E test, just to make sure it stays working.

@@ -86,6 +91,13 @@ function ChartTable({

const [chartFilter, setChartFilter] = useState('Mine');

useEffect(() => {
const filter = getFromLocalStorage('chart', null);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should use more specific names for the local storage keys than activity and chart since these names need to be shared globally across the entire application. Perhaps use homepage_chart_filter here.

@@ -201,6 +216,7 @@ export default function ActivityTable({
label: t('Examples'),
onClick: () => {
setActiveChild('Examples');
setInLocalStorage('activity', { activity: 'Examples' });
Copy link
Member

Choose a reason for hiding this comment

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

perhaps homepage_active_tab as the key here

@@ -69,6 +73,13 @@ function DashboardTable({
const [editModal, setEditModal] = useState<Dashboard>();
const [dashboardFilter, setDashboardFilter] = useState('Mine');

useEffect(() => {
const filter = getFromLocalStorage('dashboard', null);
Copy link
Member

Choose a reason for hiding this comment

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

homepage_dashboard_filter?

Copy link
Member

Choose a reason for hiding this comment

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

Also, these keys could be extracted into constants

Copy link
Member

Choose a reason for hiding this comment

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

👍 on keeping these in a constants file or using an enum. It’ll make it much easier to add a new key without risk of collision with an existing one

@@ -71,6 +75,7 @@ function ChartTable({
false,
);

useEffect(() => {});
Copy link
Member

Choose a reason for hiding this comment

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

Empty useEffect?

@pkdotson pkdotson mentioned this pull request May 25, 2021
8 tasks
@rusackas rusackas deleted the sticky-tabs-hp branch May 25, 2021 21:38
@junlincc junlincc removed the demo label May 28, 2021
@kgabryje
Copy link
Member

I found a weird bug on Dashboards panel - I don't have any favourite dashboards, but when I reload, dashboards from "Mine" tab show up in "Favourite" tab

Screen.Recording.2021-05-28.at.15.13.44.mov

cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
* inital commit

* lint

* fix loading state for edit activity

* add dep

* lint fix

* add condition

* lint being lint
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
* inital commit

* lint

* fix loading state for edit activity

* add dep

* lint fix

* add condition

* lint being lint
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* inital commit

* lint

* fix loading state for edit activity

* add dep

* lint fix

* add condition

* lint being lint
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io release:note size/M 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants