feat(style): hide dashboard header by url parameter by simcha90 · Pull Request #12918 · apache/superset

simcha90 · 2021-02-03T14:24:23Z

SUMMARY

Hide dashboard header with name and edit button dependent on url parameter standalone
e.g. http://localhost:8088/superset/dashboard/11/?standalone=true
Here supported options (according proposal of @ktmud ):
a. standalone=true (for backward compatibility) or standalone=1 hide only navbar
b. standalone=2 hide navbar and title
c. standalone=false (for backward compatibility) or no param show all
Add global constants for url params

Reason for PR: When dashboard is embedded as standalone there is also a use-case to hide the dashboard header

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-02-07.at.10.31.56.mov

TEST PLAN

Test according 1. flows
Test filter bar of native filters working as expected

ADDITIONAL INFORMATION

codecov-io · 2021-02-03T15:47:40Z

Codecov Report

Merging #12918 (7c2bf4d) into master (2ce7982) will increase coverage by 8.83%.
The diff coverage is 65.38%.

@@            Coverage Diff             @@
##           master   #12918      +/-   ##
==========================================
+ Coverage   53.06%   61.89%   +8.83%     
==========================================
  Files         489      547      +58     
  Lines       17314    20173    +2859     
  Branches     4482     5282     +800     
==========================================
+ Hits         9187    12486    +3299     
+ Misses       8127     7475     -652     
- Partials        0      212     +212

Flag	Coverage Δ
cypress	`?`
javascript	`61.89% <65.38%> (?)`

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

Impacted Files	Coverage Δ
...ontend/src/components/ListViewCard/ImageLoader.tsx	`86.36% <0.00%> (+11.36%)`	⬆️
...set-frontend/src/components/URLShortLinkButton.jsx	`100.00% <ø> (ø)`
...src/dashboard/components/HeaderActionsDropdown.jsx	`54.79% <ø> (-12.78%)`	⬇️
...end/src/dashboard/components/StickyVerticalBar.tsx	`50.00% <ø> (-50.00%)`	⬇️
...rontend/src/explore/components/EmbedCodeButton.jsx	`80.76% <ø> (ø)`
...ntend/src/explore/components/ExploreChartPanel.jsx	`16.17% <ø> (-67.41%)`	⬇️
...nd/src/explore/components/ExploreViewContainer.jsx	`2.43% <0.00%> (-77.07%)`	⬇️
superset-frontend/src/utils/common.js	`87.27% <ø> (+68.43%)`	⬆️
...perset-frontend/src/views/CRUD/chart/ChartList.tsx	`71.65% <ø> (-2.66%)`	⬇️
...rontend/src/views/CRUD/dashboard/DashboardCard.tsx	`75.67% <ø> (-0.33%)`	⬇️
... and 477 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 3e0681b...7c2bf4d. Read the comment docs.

junlincc · 2021-02-03T23:09:07Z

thanks for the PR @simcha90 ! User will still be able to add filters after hiding header and edit button since the actions take place in the view mode. is this intentional?
@mihir174 I think this is a valid user need - sharing a dashboard to those who should not be editing it. this solution to me is a bit "hacky" though. any idea?
from Product standpoint, this is a good start, if the code looks good, let's let it in and iterate on the the UX ✅
feature works as expected! ✅

mihir174 · 2021-02-04T00:32:22Z

Agreed that this is a bit hacky but it's a good first step. We need to think about security/view permission configurations on embed links.
I like the way Google, for example, deals with link sharing:

ktmud · 2021-02-04T00:39:45Z

Why not call the parameter standalone, too, just to be consistent with charts?

simcha90 · 2021-02-04T08:13:54Z

Why not call the parameter standalone, too, just to be consistent with charts?

standalone - hides only top menu bar
hide_dashboard_hide - hides only header with edit button

@ktmud I think it dependent on user needs we need give both options to manage dashboard view

ktmud · 2021-02-04T09:31:03Z

Why would you want to keep Superset navs while hiding dashboard title? Wouldn't users be able to navigate to other Superset pages in the embedded dashboard? I'm finding it hard to imagine a scenario where this would be desired.

amitmiran137 · 2021-02-04T09:50:53Z

@ktmud I agree that the use-case of standalone=false and hide_dashboard_hide=true does not exist
but there are 2 cases that do exist

standalone=true & hide_dashboard_hide=false
standalone =true & hide_dashboard_hide=true

I can agree that by just setting hide_dashboard_hide=true we can enable standalone but not sure we must enforce it
WDYT?

ktmud · 2021-02-04T18:07:17Z

How about we introduce two "standalone modes"?

standelone=true or standlone=1: hide navs only
standalone=2: hide navs and title

Let's not give users the choice to hide just the dashboard title. If the request comes back up with a reasonable business need to support it, then we can add it as standlone=3.

simcha90 · 2021-02-07T08:44:22Z

How about we introduce two "standalone modes"?

standelone=true or standlone=1: hide navs only

standalone=2: hide navs and title

Let's not give users the choice to hide just the dashboard title. If the request comes back up with a reasonable business need to support it, then we can add it as standlone=3.

done

…rset into hide_header

� Conflicts: � superset-frontend/src/explore/components/ExploreViewContainer.jsx

ktmud

Some code style suggestions, most are non-blocking.

ktmud · 2021-02-09T08:40:29Z

+export function getUrlParam(paramName: string, type: 'string'): string;
+export function getUrlParam(paramName: string, type: 'number'): number;
+export function getUrlParam(paramName: string, type: 'boolean'): boolean;
+export function getUrlParam(paramName: string, type: UrlParamType): unknown {


This feel like a useful general util. Maybe move to src/commons/utils/getUrlPram.ts?

moved it here: superset-frontend/src/utils/common.ts

ktmud

Code LGTM, just one comment about typing.

I'm also seeing this gap for native filters. Is this expected?

ktmud · 2021-02-09T21:56:31Z

+  schema: any;
+  autorun: any;
+  sql: any;
+}) {


superset/superset-frontend/src/SqlLab/components/ShareSqlLabQuery.jsx

Lines 34 to 41 in 9997abe

queryEditor: PropTypes.shape({

dbId: PropTypes.number,

title: PropTypes.string,

schema: PropTypes.string,

autorun: PropTypes.bool,

sql: PropTypes.string,

remoteId: PropTypes.number,

}).isRequired,

Maybe make the typing consistent with here?

To avoid having to update typing for all other util functions, you may also create getParamFromQuery as its own file. I think in general we should welcome smaller util files so it's easier to maintain and search (to find a specific util function you can just search by file name instead of search file content).

👍 did as second proposal, created file for urlUtils

simcha90 · 2021-02-10T10:16:53Z

@ktmud 👍 good catch about UI bug, fixed

…rset into hide_header

ktmud

LGTM with one last suggestion.

Thanks for all the iterations!

ktmud · 2021-02-10T18:22:31Z

cc @junlincc if you want to do a final round of QA & product sign-off.

villebro

A few non blocking nits

* feat(native-filters): hide dashboard header bu url parameter * lint: fix lint * test: add tests * test: fix test * refactor: upgrade standalone param * fix: pre-commit and extract to method is_standalone_mode * test: fix tests * test: fix tests * fix: fix standalone statement * refactor: fix CR notes * chore: pre-commit * fix: fix sticky tabs + update CR notes * lint: fix lint * lint: fix lint * fix: fix CR notes * fix: fix CR notes * lint: fix lint * refactor: fix cr notes Co-authored-by: amitmiran137 <amit.miran@nielsen.com>

* master: (30 commits) refactor(native-filters): decouple params from filter config modal (first phase) (apache#13021) fix(native-filters): set currentValue null when empty (apache#13000) Custom superset_config.py + secret envs (apache#13096) Update http error code from 400 to 403 (apache#13061) feat(native-filters): add storybook entry for select filter (apache#13005) feat(native-filters): Time native filter (apache#12992) Force pod restart on config changes (apache#13056) feat(cross-filters): add cross filters (apache#12662) fix(explore): Enable selecting an option not included in suggestions (apache#13029) Improves RTL configuration (apache#13079) Added a note about the ! prefix for breaking changes to CONTRIBUTING.md (apache#13083) chore: lock down npm to v6 (apache#13069) fix: API tests, make them possible to run independently again (apache#13076) fix: add config to disable dataset ownership on the old api (apache#13051) add required * indicator to message content/notif method (apache#12931) fix: Retroactively add granularity param to charts (apache#12960) fix(ci): multiline regex in change detection (apache#13075) feat(style): hide dashboard header by url parameter (apache#12918) fix(explore): pie chart label bugs (apache#13052) fix: Disabled state button transition time (apache#13008) ...

* feat(native-filters): hide dashboard header bu url parameter * lint: fix lint * test: add tests * test: fix test * refactor: upgrade standalone param * fix: pre-commit and extract to method is_standalone_mode * test: fix tests * test: fix tests * fix: fix standalone statement * refactor: fix CR notes * chore: pre-commit * fix: fix sticky tabs + update CR notes * lint: fix lint * lint: fix lint * fix: fix CR notes * fix: fix CR notes * lint: fix lint * refactor: fix cr notes Co-authored-by: amitmiran137 <amit.miran@nielsen.com>

feat(native-filters): hide dashboard header bu url parameter

2054d66

pull-request-size Bot added the size/L label Feb 3, 2021

simcha90 changed the title ~~feat(native-filters): hide dashboard header bu url parameter~~ feat(style): hide dashboard header bu url parameter Feb 3, 2021

simcha90 changed the title ~~feat(style): hide dashboard header bu url parameter~~ feat(style): hide dashboard header by url parameter Feb 3, 2021

simcha90 added 2 commits February 3, 2021 17:18

lint: fix lint

ee7cae6

test: add tests

ac4ac8c

test: fix test

a11e8ad

junlincc added viz:dashboard:customize hold:testing! On hold for testing requires:design-review Requires input/approval from a Designer labels Feb 3, 2021

junlincc requested a review from suddjian February 3, 2021 23:13

junlincc removed the hold:testing! On hold for testing label Feb 3, 2021

refactor: upgrade standalone param

173b85c

amitmiran137 and others added 6 commits February 7, 2021 11:14

fix: pre-commit and extract to method is_standalone_mode

7dd7422

test: fix tests

788f1c8

Merge branch 'hide_header' of github.com:simchaNielsen/incubator-supe…

f3b3da0

…rset into hide_header

test: fix tests

385088d

fix: fix standalone statement

b4d31a1

Merge branch 'master' of github.com:apache/superset into hide_header

14b7c5b

� Conflicts: � superset-frontend/src/explore/components/ExploreViewContainer.jsx

ktmud reviewed Feb 9, 2021

View reviewed changes

refactor: fix CR notes

1c7eef3

chore: pre-commit

5501501

ktmud reviewed Feb 9, 2021

View reviewed changes

fix: fix sticky tabs + update CR notes

cb693ab

simcha90 added 4 commits February 10, 2021 12:17

Merge branch 'master' of github.com:apache/superset into hide_header

909ad9a

Merge branch 'hide_header' of github.com:simchaNielsen/incubator-supe…

252d268

…rset into hide_header

lint: fix lint

3ad9c36

lint: fix lint

6a4de67

ktmud approved these changes Feb 10, 2021

View reviewed changes

Comment thread superset-frontend/src/explore/exploreUtils.js Outdated

simcha90 added 3 commits February 11, 2021 09:29

fix: fix CR notes

3ac5750

fix: fix CR notes

c02286f

lint: fix lint

d8c89a6

villebro approved these changes Feb 11, 2021

View reviewed changes

Comment thread superset-frontend/spec/javascripts/explore/utils_spec.jsx Outdated

Comment thread superset-frontend/spec/javascripts/dashboard/util/getDashboardUrl_spec.js Outdated

Comment thread superset-frontend/spec/javascripts/explore/components/EmbedCodeButton_spec.jsx Outdated

refactor: fix cr notes

7c2bf4d

villebro merged commit c5781cd into apache:master Feb 11, 2021

villebro mentioned this pull request Jun 4, 2021

fix: toggle fullscreen on the dashboard #14979

Merged

8 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 First shipped in 1.2.0 labels Mar 12, 2024

	queryEditor: PropTypes.shape({
	dbId: PropTypes.number,
	title: PropTypes.string,
	schema: PropTypes.string,
	autorun: PropTypes.bool,
	sql: PropTypes.string,
	remoteId: PropTypes.number,
	}).isRequired,

Conversation

simcha90 commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

Uh oh!

codecov-io commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

junlincc commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mihir174 commented Feb 4, 2021

Uh oh!

ktmud commented Feb 4, 2021

Uh oh!

simcha90 commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktmud commented Feb 4, 2021

Uh oh!

amitmiran137 commented Feb 4, 2021

Uh oh!

ktmud commented Feb 4, 2021

Uh oh!

simcha90 commented Feb 7, 2021

Uh oh!

ktmud left a comment

Choose a reason for hiding this comment

Uh oh!

ktmud Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

simcha90 Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ktmud left a comment

Choose a reason for hiding this comment

Uh oh!

ktmud Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

simcha90 Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

simcha90 commented Feb 10, 2021

Uh oh!

ktmud left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ktmud commented Feb 10, 2021

Uh oh!

villebro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

simcha90 commented Feb 3, 2021 •

edited

Loading

codecov-io commented Feb 3, 2021 •

edited

Loading

junlincc commented Feb 3, 2021 •

edited

Loading

simcha90 commented Feb 4, 2021 •

edited

Loading