Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

fix: set correct favicon from config for login and FAB list views #21498

Merged
merged 2 commits into from
Sep 19, 2022
Merged

fix: set correct favicon from config for login and FAB list views #21498

merged 2 commits into from
Sep 19, 2022

Conversation

mayurnewase
Copy link
Contributor

@mayurnewase mayurnewase commented Sep 17, 2022 •
edited
Loading

SUMMARY

fixes: #21494

Most of the views use basic.html, but base.html is still used by login and FAB model list views, so updated its favicon to match basic.html

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue: Branding favicon is not used on all page #21494
  • 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

@mayurnewase mayurnewase changed the title fix: set correct favicon from config for login and list views fix: set correct favicon from config for login and FAB list views Sep 17, 2022
mayurnewase
mayurnewase commented Sep 17, 2022
View reviewed changes
superset/templates/superset/base.html
@@ -21,7 +21,15 @@

{% block head_css %}
{{ super() }}
<link rel="icon" type="image/png" href="{{ assets_prefix }}/static/assets/images/favicon.png">
{% set favicons = appbuilder.app.config['FAVICONS'] %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

copied from https://github.com/apache/superset/blob/master/superset/templates/superset/basic.html#L38

@codecov
Copy link

codecov bot commented Sep 17, 2022 •
edited
Loading

Codecov Report

Merging #21498 (5c348e6) into master (6e8cad3) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #21498      +/-   ##
==========================================
+ Coverage   66.59%   66.61%   +0.02%     
==========================================
  Files        1791     1791              
  Lines       68554    68594      +40     
  Branches     7319     7319              
==========================================
+ Hits        45653    45695      +42     
+ Misses      21008    21006       -2     
  Partials     1893     1893
Flag Coverage Δ
hive 53.08% <ø> (+0.01%) ⬆️
mysql 78.20% <ø> (+0.03%) ⬆️
postgres 78.27% <ø> (+0.03%) ⬆️
presto 52.98% <ø> (+0.01%) ⬆️
python 81.41% <ø> (+0.02%) ⬆️
sqlite 76.76% <ø> (+0.03%) ⬆️
unit 51.05% <ø> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
superset/common/query_context_processor.py 88.51% <0.00%> (-0.29%) ⬇️
superset/viz.py 58.17% <0.00%> (ø)
superset/connectors/sqla/models.py 90.60% <0.00%> (ø)
superset/utils/core.py 90.32% <0.00%> (+0.38%) ⬆️
superset/views/core.py 76.04% <0.00%> (+0.48%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zhaoyongjie zhaoyongjie self-requested a review September 19, 2022 07:23
zhaoyongjie
zhaoyongjie approved these changes Sep 19, 2022
View reviewed changes
Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

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

nice fix! lgtm

@zhaoyongjie zhaoyongjie merged commit b29e7e7 into apache:master Sep 19, 2022
eschutho pushed a commit that referenced this pull request Sep 20, 2022
@mayurnewase @eschutho
fix: set correct favicon from config for login and FAB list views (#2…
0da708e 
…1498)

(cherry picked from commit b29e7e7)
AAfghahi pushed a commit that referenced this pull request Oct 5, 2022
@mayurnewase @AAfghahi
fix: set correct favicon from config for login and FAB list views (#2…
025ddbc 
…1498)

(cherry picked from commit b29e7e7)
AAfghahi pushed a commit that referenced this pull request Oct 6, 2022
@mayurnewase @AAfghahi
fix: set correct favicon from config for login and FAB list views (#2…
dfcb66a 
…1498)

(cherry picked from commit b29e7e7)
Fahrenheit35 pushed a commit to Fahrenheit35/superset that referenced this pull request Nov 11, 2022
@mayurnewase @Fahrenheit35
fix: set correct favicon from config for login and FAB list views (ap…
b3219ef 
…ache#21498)

(cherry picked from commit b29e7e7)
@mistercrunch mistercrunch added 🍒 2.0.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@zhaoyongjie zhaoyongjie zhaoyongjie approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S v2.0 v2.0.1 🍒 2.0.1 🚢 2.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Branding favicon is not used on all page
4 participants
@mayurnewase @zhaoyongjie @mistercrunch @eschutho