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.

Sign up for GitHub

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

feat: TV3-181 allow remote LOGO and FAVICON #783

Merged
merged 7 commits into from
Jan 30, 2024
Merged

feat: TV3-181 allow remote LOGO and FAVICON #783

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f833850
feat: TV3-181 allow remote LOGO and FAVICON
wesleyboar Jan 9, 2024
ceb26d3
fix: TV3-181 remote LOGO FAVICON syntax error
wesleyboar Jan 9, 2024
a81484c
fix: TV3-181 template syntax bug & reduce diff
wesleyboar Jan 9, 2024
770ac9d
fix: TV3-181 simplify inaccurate comparison
wesleyboar Jan 9, 2024
8d00777
fix: TV3-181 new TACC_LOGO, deprecate LOGO
wesleyboar Jan 11, 2024
36b025a
fix: TV3-181 TACC_FAVICON, deprecate FAVICON+LOGO
wesleyboar Jan 11, 2024
dd3ad19
Merge branch 'main' into feat/TV3-181-add-ptdatax-resources
wesleyboar Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion taccsite_cms/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,8 @@ def gettext(s): return s
]

FAVICON = {
"img_file_src": "site_cms/img/favicons/favicon.ico"
"img_file_src": "site_cms/img/favicons/favicon.ico",
"is_remote": False
}

########################
Expand Down
2 changes: 1 addition & 1 deletion taccsite_cms/templates/assets_custom.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

<!-- Custom Site Assets: Favicon. -->
{% with settings.FAVICON as favicon %}
<link rel="icon" href="{% static favicon.img_file_src %}" type="image/x-icon" />
<link rel="icon" href="{% if favicon.is_remote %}{{ favicon.img_file_src }}{% else %}{% static favicon.img_file_src %}{% endif %}" type="image/x-icon" />
Copy link
Member

Choose a reason for hiding this comment

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

Would you be able to make this change for Core-Portal as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but I'm worried about doing it without answering questions first:

  1. Why does Portal use {% static %} as '/core/static/'?
  2. Would it be confusing or clearer for CMS and Portal to have the same STATIC_URL = '/static/' setting?
  3. Should Core-Portal refactor to, like TUP, use only one Django instance?

I am still recovering from flu, so apologies if I am dense.

Copy link
Member

Choose a reason for hiding this comment

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

1 and 2: There is the risk of file collision if both portal and cms use the same static directory. Likelihood does seem low. Is there an advantage to both using the same static url?
3: Can you expand on this? I'm not sure what you're referring to exactly

Copy link
Member Author

Choose a reason for hiding this comment

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

1. & 2. I can't think of one.
3. Unrelated topic I was forcing into the conversation through logic I did not expose. Please never mind for this topic.

Yes, I can do it.

On Portal, would the value /static/... (to load from Core-CMS) use is_remote = TRUE, because it's not using Core-Portal's {% static %}?

Copy link
Member

Choose a reason for hiding this comment

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

On Portal, would the value /static/... (to load from Core-CMS) use is_remote = TRUE, because it's not using Core-Portal's {% static %}?

Ah I see, the portal already kinda handles this in that it doesn't default to using % static %. Think it's still needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot about this. Back now. And reading it without the flu fogging my brain.

I think Portal does not need {% static %} option for favicon. It works for remote and CMS assets with its code as is. To update Portal would be for consistency. A welcome change, but a rabbit hole. The divergence occurred and is great now.

Details (for those who didn't read the thread)

Adding {% static %} and is_remote option to Portal adds complexity. The Portal already assumes favicon is not a "local" asset, thus assumes it will be given a non-Portal-static-asset URL. If existing hardcoded /static/ values concern us, we can add something like CMS_STATIC_DIR in default settings.

https://github.com/TACC/Core-Portal/blob/v3.5.0/server/portal/templates/base.html#L11-L13

    {% if settings.PORTAL_ICON_FILENAME %}
    <link rel="icon" href="{{ settings.PORTAL_ICON_FILENAME }}">
    {% else %}

Copy link
Member Author

Choose a reason for hiding this comment

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

@taoteg will test this theory on DigitalRocks. 😀

{% endwith %}


Expand Down
2 changes: 1 addition & 1 deletion taccsite_cms/templates/header_logo.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
{% with filename=logo|index:1 selectors=logo|index:2 targeturl=logo|index:3 targettype=logo|index:4 accessibility=logo|index:5 corstype=logo|index:6 visibility=logo|index:7 %}
{% if visibility == "True" %}
<a id="header-logo" class="navbar-brand {{className}}" href="{{ targeturl }}" target="{{ targettype }}">
<img class="portal-logo {{ selectors }}" src="{% static filename %}" crossorigin="{{ corstype }}" alt="{{ accessibility }}" >
<img class="portal-logo {{ selectors }}" src="{% if "://" in filename %}{{ filename }}{% else %}{% static filename %}{% endif %}" crossorigin="{{ corstype }}" alt="{{ accessibility }}" />
wesleyboar marked this conversation as resolved.
Show resolved Hide resolved
</a>
{% endif %}
{% endwith %}
Expand Down