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

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

Merged
merged 2 commits into from
Oct 4, 2021

Conversation

pkdotson
Copy link
Member

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
Copy link

codecov bot commented Sep 13, 2021

Codecov Report

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

Impacted file tree graph

@@           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
Copy link

graceguo-supercat commented Sep 13, 2021

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
Copy link
Member

@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.
Screen-Recording-2021-09-13-at-5

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
Copy link

graceguo-supercat commented Sep 14, 2021

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
Copy link
Member Author

/testenv up

@pkdotson
Copy link
Member Author

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
Copy link
Contributor

@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
Copy link
Member

@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
Copy link
Member

jinghua-qa commented Sep 14, 2021

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
Copy link

graceguo-supercat commented Sep 15, 2021

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
Copy link
Member Author

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.
Screen Shot 2021-09-15 at 3 00 24 PM

Copy link

@graceguo-supercat graceguo-supercat left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for the work!

@pkdotson
Copy link
Member Author

pkdotson commented Oct 4, 2021

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2021

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

@pkdotson pkdotson merged commit 50ad84b into apache:master Oct 4, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2021

Ephemeral environment shutdown and build artifacts deleted.

opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
@mistercrunch 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
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 size/L 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[dashboard edit] can we make tool menu show up for header when hover over
4 participants