fix: toggle fullscreen on the dashboard by zhaoyongjie · Pull Request #14979 · apache/superset

zhaoyongjie · 2021-06-04T03:38:55Z

SUMMARY

closes: #14961
This is a regression bug. currently, the user can't make the dashboard to fullscreen mode, this PR fixed it.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before

dashboard.before.mp4

after

dashboard.after.mp4

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue: [dashboard] 'toggle to full screen' option in 3 dot menu do not turn dashboard to full screen #14961
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

codecov · 2021-06-04T03:56:49Z

Codecov Report

Merging #14979 (d5e8e5d) into master (004a6d9) will decrease coverage by 0.23%.
The diff coverage is 100.00%.

❗ Current head d5e8e5d differs from pull request most recent head 5265c79. Consider uploading reports for the commit 5265c79 to get more accurate results

@@            Coverage Diff             @@
##           master   #14979      +/-   ##
==========================================
- Coverage   77.61%   77.38%   -0.24%     
==========================================
  Files         965      965              
  Lines       49503    49504       +1     
  Branches     6259     6259              
==========================================
- Hits        38422    38308     -114     
- Misses      10881    10996     +115     
  Partials      200      200

Flag	Coverage Δ
hive	`?`
javascript	`72.48% <100.00%> (+<0.01%)`	⬆️
presto	`?`

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

Impacted Files	Coverage Δ
.../components/Header/HeaderActionsDropdown/index.jsx	`68.42% <ø> (ø)`
...rontend/src/filters/components/Range/buildQuery.ts	`40.00% <ø> (ø)`
...ontend/src/filters/components/Select/buildQuery.ts	`100.00% <ø> (ø)`
...rontend/src/visualizations/FilterBox/FilterBox.jsx	`56.60% <ø> (ø)`
...c/filters/components/Select/SelectFilterPlugin.tsx	`81.72% <100.00%> (+0.19%)`	⬆️
superset/db_engines/hive.py	`0.00% <0.00%> (-82.15%)`	⬇️
superset/db_engine_specs/hive.py	`70.32% <0.00%> (-17.08%)`	⬇️
superset/db_engine_specs/presto.py	`83.36% <0.00%> (-6.53%)`	⬇️
superset/views/database/mixins.py	`81.03% <0.00%> (-1.73%)`	⬇️
superset/connectors/sqla/models.py	`88.15% <0.00%> (-1.66%)`	⬇️
... and 3 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 004a6d9...5265c79. Read the comment docs.

villebro · 2021-06-04T05:15:14Z

This comes down to taste, but I think the double bang could be done directly here (seems clear enough).

the "negation" is needed here, the hasStandalone has been boolean type.

Right, gotcha 👍

villebro · 2021-06-04T05:22:40Z

standalone can actually be called with the value 2, in which case it removes the title:

So perhaps number | boolean could be a good solution here, as it appears to work as expected with values undefined, true, false, 0, 1 and 2?

standalone can actually be called with the value 2, in which case it removes the title:

So perhaps number | boolean could be a good solution here, as it appears to work as expected with values undefined, true, false, 0, 1 and 2?

I know this, but I don't think it's a good design. When we remove the header there is no chance to go back. (users need to remove/change standalone parameter from the URL)

I'm thinking we should introduce a new parameter for handle "remove header", instead of using int to sign status, and introduce a button(or icon) allow users to go back.

I'm fine with that approach - I think this was discussed to some degree on the PR that introduced it: #12918

Thanks for your point out.

villebro

LGTM with minor stylistic alternative to consider

villebro · 2021-06-04T09:48:34Z

+        const hasStandalone = !!getUrlParam(URL_PARAMS.standalone);
        const url = getDashboardUrl(
          window.location.pathname,
          getActiveFilters(),
          window.location.hash,
-          getUrlParam(URL_PARAMS.standalone),
+          !hasStandalone,


I suppose it would also be correct to do

!getUrlParam(URL_PARAMS.standalone)

here?

right, but I feel weird to do "negation" after returning a number type or null type.

github-actions · 2021-06-04T20:20:02Z

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

* upstream/master: (68 commits) fix typos (apache#14950) docs: fix custom oauth config (apache#14997) fix: apply template_params on external_metadata (apache#14996) fix: toggle fullscreen on the dashboard (apache#14979) feat(native-filters): add markers and number formatter + simple tests (apache#14981) fix(native-filters): Fix "undefined" error after editing a filter (apache#14984) fix(native-filters): remove implied fetch predicate (apache#14982) fix(native-filters): update cascaded filter state on change (apache#14980) fix(filter box): replace freeform where clause with ilike (apache#14900) refactor: Convert TableElement.jsx component from class to functional with hooks (apache#14830) fix: renamed sqllab filters to _filters (apache#14971) feat(native-filters): apply cascading without instant filtering (apache#14966) chore: bump superset-ui to 0.17.53 (apache#14968) fix(native-filters): cascading filters not rendering in tab (apache#14964) feat: add type_generic and is_dttm to table metadata (apache#14863) additional safeguard (apache#14953) feat: Adding FORCE_SSL as feature flag in config.py (apache#14934) feat(dashboard/native-filters): Hide filters out of scope of current tab (apache#14933) fix: time parser truncate to first day of year/month (apache#14945) fix: is_temporal should overwrite is_dttm (apache#14894) ...

pull-request-size Bot added the size/S label Jun 4, 2021

zhaoyongjie requested review from simcha90 and villebro June 4, 2021 03:39

nytai requested a review from suddjian June 4, 2021 03:54

villebro reviewed Jun 4, 2021

View reviewed changes

fix: toggle fullscreen on the dashboard

526b5a5

zhaoyongjie force-pushed the fix_dashboard_fullscreen branch from 09b6373 to 526b5a5 Compare June 4, 2021 06:53

pull-request-size Bot added size/XS and removed size/S labels Jun 4, 2021

fix lint

d973150

villebro approved these changes Jun 4, 2021

View reviewed changes

updates

5265c79

zhaoyongjie merged commit c8cf4b6 into apache:master Jun 4, 2021

junlincc added the bash! label Jun 4, 2021

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

fix: toggle fullscreen on the dashboard (apache#14979)

a822434

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

fix: toggle fullscreen on the dashboard (apache#14979)

f849f7e

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

fix: toggle fullscreen on the dashboard (apache#14979)

ea1ff9b

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

qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026

fix: toggle fullscreen on the dashboard (apache#14979)

87f5d32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: toggle fullscreen on the dashboard#14979

fix: toggle fullscreen on the dashboard#14979
zhaoyongjie merged 3 commits into
apache:masterfrom
zhaoyongjie:fix_dashboard_fullscreen

zhaoyongjie commented Jun 4, 2021 •

edited

Loading

Uh oh!

codecov Bot commented Jun 4, 2021 •

edited

Loading

Uh oh!

villebro Jun 4, 2021

Uh oh!

zhaoyongjie Jun 4, 2021

Uh oh!

villebro Jun 4, 2021

Uh oh!

villebro Jun 4, 2021

Uh oh!

zhaoyongjie Jun 4, 2021 •

edited

Loading

Uh oh!

villebro Jun 4, 2021

Uh oh!

zhaoyongjie Jun 4, 2021

Uh oh!

villebro left a comment

Uh oh!

villebro Jun 4, 2021

Uh oh!

zhaoyongjie Jun 4, 2021 •

edited

Loading

Uh oh!

github-actions Bot commented Jun 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

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

Uh oh!

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before

after

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

villebro Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

zhaoyongjie Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

villebro Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

villebro Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

villebro Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

zhaoyongjie Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

villebro left a comment

Choose a reason for hiding this comment

Uh oh!

villebro Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zhaoyongjie commented Jun 4, 2021 •

edited

Loading

codecov Bot commented Jun 4, 2021 •

edited

Loading

zhaoyongjie Jun 4, 2021 •

edited

Loading

zhaoyongjie Jun 4, 2021 •

edited

Loading