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: Add home link to navbar #11851

Merged
merged 3 commits into from Dec 1, 2020

Conversation

agatapst
Copy link
Contributor

SUMMARY

This PR adds new element to the top navbar - HOME link redirecting to the homepage.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Navbar before:
image

Navbar after:
image

TEST PLAN

First, run command superset init to create default roles and permissions (navbar varies for different types of users, but home link should be seen by every user.) Then verify manually by visiting some application page.

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

cc @junlincc @rusackas

@agatapst agatapst changed the title Chore/add home to navbar chore: Add home to navbar Nov 30, 2020
@codecov-io
Copy link

codecov-io commented Nov 30, 2020

Codecov Report

Merging #11851 (258e5b8) into master (9dd33d5) will decrease coverage by 2.99%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11851      +/-   ##
==========================================
- Coverage   66.71%   63.71%   -3.00%     
==========================================
  Files         917      917              
  Lines       44700    44735      +35     
  Branches     4235     4236       +1     
==========================================
- Hits        29820    28502    -1318     
- Misses      14762    16056    +1294     
- Partials      118      177      +59     
Flag Coverage Δ
cypress ?
javascript 62.94% <ø> (-0.02%) ⬇️
python 64.15% <50.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
superset-frontend/src/components/Menu/Menu.tsx 63.51% <ø> (-31.09%) ⬇️
superset/app.py 81.20% <50.00%> (-0.24%) ⬇️
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%) ⬇️
... and 141 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 9dd33d5...258e5b8. Read the comment docs.

@agatapst agatapst changed the title chore: Add home to navbar chore: Add home link to navbar Nov 30, 2020
@junlincc
Copy link
Member

@eugeniamz

@junlincc junlincc added the rush! Requires immediate attention label Nov 30, 2020
@etr2460
Copy link
Member

etr2460 commented Nov 30, 2020

I'm curious, isn't this redundant when you consider the Superset logo also goes to the welcome page? @junlincc what was the design/product rationale around this change?

@willbarrett
Copy link
Member

@etr2460 it's possible to override the URL and logo in Superset, so this provides an additional way to get to the Superset home page when that configuration is leveraged.

@nytai
Copy link
Member

nytai commented Nov 30, 2020

@etr2460 this is in case someone has the LOGO_TARGET_PATH flag set, there's no other way to navigate to the home screen.

@junlincc
Copy link
Member

junlincc commented Nov 30, 2020

@etr2460
currently, companies that forked Superset, don't have any direct entry points to go back to the landing page/home page within the product. since the logo usually takes users to the workspaces or their own company home page.

@etr2460
Copy link
Member

etr2460 commented Nov 30, 2020

Thanks for the clarification! Maybe we only show the new menu option if the LOGO_TARGET_PATH is set then?

@agatapst
Copy link
Contributor Author

agatapst commented Nov 30, 2020

@junlincc thanks for your reply
@etr2460 that's good idea, thanks for the tip!

To test option with logo target path set, LOGO_TARGET_PATH can be found in config.py file. It can be changed to i.e. LOGO_TARGET_PATH = 'https://superset.apache.org/'. Then Home link will be shown.

@junlincc junlincc self-requested a review November 30, 2020 21:03
Copy link
Member

@etr2460 etr2460 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 addressing my comment!

@rusackas rusackas merged commit 44e80e0 into apache:master Dec 1, 2020
@junlincc junlincc removed the rush! Requires immediate attention label Mar 18, 2021
@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/S 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants