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

fix: Change url & add check for active item in menu #11915

Merged
merged 4 commits into from
Dec 12, 2020

Conversation

maloun96
Copy link
Contributor

@maloun96 maloun96 commented Dec 3, 2020

SUMMARY

Add underline for Saved Query menu.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before
image
After
image

TEST PLAN

Log In
Navigate to SQL Lab -> Saved Queries, observer SQL Lab does not have underline border

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API
  • Fix A underline issue #11874

@maloun96 maloun96 changed the title Change url & add check for active item in menu Fix: Change url & add check for active item in menu Dec 3, 2020
@maloun96 maloun96 changed the title Fix: Change url & add check for active item in menu fix: Change url & add check for active item in menu Dec 3, 2020
@pull-request-size pull-request-size bot added size/M and removed size/S labels Dec 3, 2020
@codecov-io
Copy link

codecov-io commented Dec 3, 2020

Codecov Report

Merging #11915 (4b563ac) into master (4666445) will decrease coverage by 3.37%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11915      +/-   ##
==========================================
- Coverage   67.12%   63.75%   -3.38%     
==========================================
  Files         915      928      +13     
  Lines       44537    45045     +508     
  Branches     4235     4309      +74     
==========================================
- Hits        29897    28717    -1180     
- Misses      14526    16151    +1625     
- Partials      114      177      +63     
Flag Coverage Δ
cypress ?
javascript 63.18% <ø> (+0.22%) ⬆️
python 64.08% <ø> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupColors.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/chart/ChartContainer.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/reducers/index.js 0.00% <0.00%> (-100.00%) ⬇️
...et-frontend/src/dashboard/containers/Dashboard.jsx 0.00% <0.00%> (-100.00%) ⬇️
...t-frontend/src/dashboard/containers/SliceAdder.jsx 0.00% <0.00%> (-100.00%) ⬇️
... and 202 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 4666445...4b563ac. Read the comment docs.

Copy link
Member

@junlincc junlincc left a comment

Choose a reason for hiding this comment

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

the issue here is that the underline is not showing up consistently, it should show or not show the cyan underline, when hovering all the tabs and menu. my suggestion is to remove the underline completely, which is currently only showing on SQL Lab editor.

@pull-request-size pull-request-size bot added size/XS and removed size/M labels Dec 3, 2020
@maloun96
Copy link
Contributor Author

maloun96 commented Dec 3, 2020

@junlincc done. Now underline is removed

@rusackas
Copy link
Member

rusackas commented Dec 4, 2020

@junlincc this seems to be fixing a smaller point of a bigger regression. The global nav used to have various hover effects including a background color and a neat little animated/expanding underline. Any idea when that disappeared entirely?

@junlincc
Copy link
Member

junlincc commented Dec 5, 2020

yea, antd tabs do offer underlines .... need to pull 0.37 to see how it used to look.

@rusackas
Copy link
Member

rusackas commented Dec 5, 2020

There are probably a bunch of CSS selectors in Emotion styles that are going "unheard" since they're trying to apply styles to the old NavItem components rather than the new ones, if those got migrated to antD.

@junlincc
Copy link
Member

junlincc commented Dec 7, 2020

Looks like the background color is still there but only on Data and SQL Lab tabs, which have the dropdown menu. only sql editor has the underline left. but even that, this behavior is not consistent.

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

Ok, we have other UI pizzaz to bring back, but I think this is a step in the right direction.

@rusackas rusackas merged commit 90ed326 into apache:master Dec 12, 2020
@rusackas rusackas mentioned this pull request Dec 13, 2020
6 tasks
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.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 size/XS 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants