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(native-filter): Hide native filters #14784

Merged
merged 13 commits into from
May 31, 2021
Merged

Conversation

simcha90
Copy link
Contributor

@simcha90 simcha90 commented May 24, 2021

SUMMARY

For Thumbnails or Alerts and Reports we need to hide filters bar on first loading

if we pass show_filters=false in url on first loading filter bar will be collapsed even if there are some filters

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

image

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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
Copy link

codecov bot commented May 24, 2021

Codecov Report

Merging #14784 (ef33f5d) into master (e466066) will increase coverage by 0.00%.
The diff coverage is 55.55%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #14784   +/-   ##
=======================================
  Coverage   77.61%   77.62%           
=======================================
  Files         963      963           
  Lines       49246    49246           
  Branches     6197     6198    +1     
=======================================
+ Hits        38224    38226    +2     
+ Misses      10821    10820    -1     
+ Partials      201      200    -1     
Flag Coverage Δ
javascript 72.49% <55.55%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
superset-frontend/src/constants.ts 100.00% <ø> (ø)
superset-frontend/src/dashboard/actions/hydrate.js 1.72% <0.00%> (-0.02%) ⬇️
.../components/Header/HeaderActionsDropdown/index.jsx 68.42% <ø> (ø)
...rontend/src/explore/components/EmbedCodeButton.jsx 80.76% <ø> (ø)
...nd/src/explore/components/ExploreViewContainer.jsx 2.15% <0.00%> (ø)
superset-frontend/src/modules/utils.js 83.33% <ø> (+30.70%) ⬆️
superset-frontend/src/utils/urlUtils.ts 53.33% <33.33%> (-8.21%) ⬇️
superset-frontend/src/components/Menu/Menu.tsx 66.66% <100.00%> (+0.40%) ⬆️
...d/components/DashboardBuilder/DashboardBuilder.tsx 87.09% <100.00%> (+0.14%) ⬆️
...set-frontend/src/dashboard/util/getDashboardUrl.ts 91.66% <100.00%> (ø)
... and 1 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 e466066...ef33f5d. Read the comment docs.

@junlincc junlincc added the dashboard:native-filters Related to the native filters of the Dashboard label May 24, 2021
@junlincc
Copy link
Member

junlincc commented May 24, 2021

@simcha90 thanks for the PR, I would really appreciate you adding some description and screenshot to the PRs, with issues properly linked. 🙏 could we also work on a fix for #14308?

@junlincc junlincc added need:qa-review Requires QA review need:screenshot need:more-info Requires more information from author hold! On hold labels May 24, 2021
@simcha90
Copy link
Contributor Author

@simcha90 thanks for the PR, I would really appreciate you adding some description and screenshot to the PRs, with issues properly linked. 🙏 could we also work on a fix for #14308?

done

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Two minor comments

superset-frontend/src/dashboard/actions/hydrate.js Outdated Show resolved Hide resolved
export function getUrlParam(paramName: string, type: 'boolean'): boolean;
export function getUrlParam(paramName: string, type: UrlParamType): unknown {
export type UrlParamType = 'string' | 'number' | 'boolean' | 'object';
export type ParamNameType = typeof URL_PARAMS[keyof typeof URL_PARAMS];
Copy link
Member

Choose a reason for hiding this comment

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

is this not the same as string? I believe it should be export type ParamNameType = keyof typeof URL_PARAMS;

Copy link
Member

Choose a reason for hiding this comment

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

This is one of those common cases that we might want to put in our wiki

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed to object so this notation protect user from put some wrong object that doesn't exists in URL_PARAMS
image

image

@junlincc junlincc added need:more-info Requires more information from author and removed hold! On hold need:more-info Requires more information from author need:qa-review Requires QA review need:screenshot labels May 26, 2021
@villebro villebro merged commit e43112c into apache:master May 31, 2021
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
* fix:fix get permission function

* feat: support showFilters

* fix: fix CR notes

* fix: fix lint issues
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
* fix:fix get permission function

* feat: support showFilters

* fix: fix CR notes

* fix: fix lint issues
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* fix:fix get permission function

* feat: support showFilters

* fix: fix CR notes

* fix: fix lint issues
@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 dashboard:native-filters Related to the native filters of the Dashboard need:more-info Requires more information from author size/M 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants