chore: refactor header menu to show in header grid component #16689

pkdotson · 2021-09-13T15:52:55Z

SUMMARY

This pr removes the hovermenu less file and refactors code to show the dashboard hover menu delete icon inside the header.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before

Screen.Recording.2021-09-13.at.8.49.48.AM.mov

after

Screen.Recording.2021-09-13.at.8.50.43.AM.mov

TESTING INSTRUCTIONS

Go to a dashboard and click on edit. Next drag a header tab to dashboard and ensure that the delete button show up on hover.

ADDITIONAL INFORMATION

[x ] Has associated issue: Fixes [dashboard edit] can we make tool menu show up for header when hover over #16424
Required feature flags:
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-09-13T16:20:31Z

Codecov Report

Merging #16689 (51b4ed1) into master (4b70d46) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #16689   +/-   ##
=======================================
  Coverage   76.88%   76.89%           
=======================================
  Files        1005     1005           
  Lines       54005    54061   +56     
  Branches     7337     7376   +39     
=======================================
+ Hits        41522    41568   +46     
- Misses      12243    12253   +10     
  Partials      240      240

Flag	Coverage Δ
javascript	`71.30% <100.00%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
...d/components/DashboardBuilder/DashboardBuilder.tsx	`90.17% <ø> (ø)`
...c/dashboard/components/gridComponents/Markdown.jsx	`83.16% <ø> (ø)`
...src/dashboard/components/gridComponents/Header.jsx	`100.00% <100.00%> (ø)`
...ontend/src/dashboard/components/menu/HoverMenu.tsx	`90.00% <100.00%> (-10.00%)`	⬇️
superset-frontend/src/logger/LogUtils.ts	`84.00% <0.00%> (-7.31%)`	⬇️
...rontend/src/components/Select/DeprecatedSelect.tsx	`82.35% <0.00%> (-3.37%)`	⬇️
...uperset-frontend/src/components/Menu/MenuRight.tsx	`91.22% <0.00%> (-1.23%)`	⬇️
...perset-frontend/src/dashboard/util/isValidChild.ts	`85.71% <0.00%> (-0.96%)`	⬇️
...ntend/src/explore/components/ExploreChartPanel.jsx	`14.28% <0.00%> (-0.72%)`	⬇️
...c/views/CRUD/data/database/DatabaseModal/index.tsx	`44.13% <0.00%> (-0.12%)`	⬇️
... and 15 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 4b70d46...51b4ed1. Read the comment docs.

graceguo-supercat · 2021-09-13T23:42:46Z

May I know why do you want to remove extra UI menu/options for dashboard header? Dashboard header and markdown component do have extra UI menu which are not available fro chart. But why is it an issue?

cc @junlincc @jinghua-qa

jinghua-qa · 2021-09-14T00:47:11Z

@graceguo-supercat, we want to make this change because of 2 reasons:
1, For header and markdown component, the trashcan icon is not shown when hovering over (shown by click), which is inconsistent behavior with the rest of other dashboard components.
2,This inconsistent behavior sometimes could cause confusion. For example, when we have a markdown in a row, when user hovering over the markdown, a trashcan icon will show because of the Row component and makes user think that icon is to delete the markdown, but clicking on that trash icon will delete the Row with all components because the trash icon is shown when hovering a row and markdown trash icon wont show until you click on it.

We got this feedback from a first time user feeback so we suggest that
1, to make trashcan icon show when hovering over for all components
2, when trashcan icon show for header and markdown, it will show in the right top conner inside the component so that it is clear to the user what is deleting.

graceguo-supercat · 2021-09-14T01:53:42Z

We got this feedback from a first time user feeback so we suggest that
1, to make trashcan icon show when hovering over for all components
2, when trashcan icon show for header and markdown, it will show in the right top conner inside the component so that it is clear to the user what is deleting.

based on your comment, this PR didn't do it correctly. it removed font-size and background menu, is it part of design?

pkdotson · 2021-09-14T14:56:35Z

/testenv up

pkdotson · 2021-09-14T14:58:11Z

We got this feedback from a first time user feeback so we suggest that
1, to make trashcan icon show when hovering over for all components
2, when trashcan icon show for header and markdown, it will show in the right top conner inside the component so that it is clear to the user what is deleting.

based on your comment, this PR didn't do it correctly. it removed font-size and background menu, is it part of design?

@graceguo-supercat font size and background menu is still there. You have to click on the grid component to have them show like you do in markdown.

github-actions · 2021-09-14T14:58:25Z

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

jinghua-qa · 2021-09-14T17:22:06Z

@pkdotson I tested in the test env, for header, it is worked as expected: trash icon is showed when hovering over, and the font size and background menu is shown when clicking on the component. But for markdown, the trash icon is not shown inside the component when hover over, it is still shown by clicking.

Screen.Recording.2021-09-14.at.10.13.04.AM.mov

jinghua-qa · 2021-09-14T17:32:16Z

We got this feedback from a first time user feeback so we suggest that
1, to make trashcan icon show when hovering over for all components
2, when trashcan icon show for header and markdown, it will show in the right top conner inside the component so that it is clear to the user what is deleting.

based on your comment, this PR didn't do it correctly. it removed font-size and background menu, is it part of design?

@graceguo-supercat I tried in the test env, the font-size and background menu will show by click in header. Is our plan to change the trashcan to show by hovering over in header and markdown sound good to you?

graceguo-supercat · 2021-09-15T00:32:08Z

before: (both header and markdown) click to show UI menu + trash icon

after:
Header: click to show UI menu, but hove to show trash icon
Markdown: click to show UI menu + trash icon.

isn't this more inconsistent? Do you really need to make this change?

pkdotson · 2021-09-16T15:58:30Z

before: (both header and markdown) click to show UI menu + trash icon

after:
Header: click to show UI menu, but hove to show trash icon
Markdown: click to show UI menu + trash icon.

isn't this more inconsistent? Do you really need to make this change?

@graceguo-supercat this has been changed as well. The delete now shows in the markdown on hover as well.

graceguo-supercat

LGTM! thanks for the work!

pkdotson · 2021-10-04T21:25:08Z

/testenv up

github-actions · 2021-10-04T21:29:33Z

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

github-actions · 2021-10-04T22:01:40Z

Ephemeral environment shutdown and build artifacts deleted.

…16689) * initial commit * add delete to markdown

initial commit

74192e4

superset-github-bot bot added the preset-io label Sep 13, 2021

pull-request-size bot added the size/L label Sep 13, 2021

pkdotson requested review from michael-s-molina, villebro, rusackas and suddjian September 14, 2021 14:54

add delete to markdown

51b4ed1

graceguo-supercat approved these changes Sep 17, 2021

View reviewed changes

pkdotson merged commit 50ad84b into apache:master Oct 4, 2021

opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021

chore: refactor header menu to show in header grid component (apache#…

3476626

…16689) * initial commit * add delete to markdown

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

chore: refactor header menu to show in header grid component (apache#…

4b2e264

…16689) * initial commit * add delete to markdown

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: refactor header menu to show in header grid component #16689

chore: refactor header menu to show in header grid component #16689

pkdotson commented Sep 13, 2021

codecov bot commented Sep 13, 2021 •

edited

Loading

graceguo-supercat commented Sep 13, 2021 •

edited

Loading

jinghua-qa commented Sep 14, 2021

graceguo-supercat commented Sep 14, 2021 •

edited by pkdotson

Loading

pkdotson commented Sep 14, 2021

pkdotson commented Sep 14, 2021

github-actions bot commented Sep 14, 2021

jinghua-qa commented Sep 14, 2021

jinghua-qa commented Sep 14, 2021 •

edited

Loading

graceguo-supercat commented Sep 15, 2021 •

edited

Loading

pkdotson commented Sep 16, 2021

graceguo-supercat left a comment

pkdotson commented Oct 4, 2021

github-actions bot commented Oct 4, 2021

github-actions bot commented Oct 4, 2021

chore: refactor header menu to show in header grid component #16689

chore: refactor header menu to show in header grid component #16689

Conversation

pkdotson commented Sep 13, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Sep 13, 2021 • edited Loading

Codecov Report

graceguo-supercat commented Sep 13, 2021 • edited Loading

jinghua-qa commented Sep 14, 2021

graceguo-supercat commented Sep 14, 2021 • edited by pkdotson Loading

pkdotson commented Sep 14, 2021

pkdotson commented Sep 14, 2021

github-actions bot commented Sep 14, 2021

jinghua-qa commented Sep 14, 2021

jinghua-qa commented Sep 14, 2021 • edited Loading

graceguo-supercat commented Sep 15, 2021 • edited Loading

pkdotson commented Sep 16, 2021

graceguo-supercat left a comment

Choose a reason for hiding this comment

pkdotson commented Oct 4, 2021

github-actions bot commented Oct 4, 2021

github-actions bot commented Oct 4, 2021

codecov bot commented Sep 13, 2021 •

edited

Loading

graceguo-supercat commented Sep 13, 2021 •

edited

Loading

graceguo-supercat commented Sep 14, 2021 •

edited by pkdotson

Loading

jinghua-qa commented Sep 14, 2021 •

edited

Loading

graceguo-supercat commented Sep 15, 2021 •

edited

Loading