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

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

Merged
merged 7 commits into from
Jan 30, 2024

Conversation

wesleyboar
Copy link
Member

@wesleyboar wesleyboar commented Jan 9, 2024

Overview

  • Allow adding remote URLs for logo and favicon.
  • Support TACC_LOGO and TACC_FAVICON.
  • Deprecate LOGO and FAVICON.

Related

Changes

  • added is_remote boolean for FAVICON settings
  • added simple protocol parse logic for LOGO filename
  • added non-static asset load for LOGO and FAVICON settings

Testing

  1. Load Core-CMS sans taccsite_cms/settings_custom.py.
  2. Verify logo is default — white "PORTAL".
  3. Verify favicon is default — white star on blue field.
  4. Load Core-CMS with taccsite_cms/settings_custom.py.
    cp taccsite_cms/settings_custom.example.py taccsite_cms/settings_custom.py
  5. Restart server.
  6. Verify logo is PTDataX's — "DATAX".
  7. Verify favicon is PTDataX's — multi-colored "X".
  8. Edit taccsite_cms/settings.py, uncomment LOGO setting.
  9. Edit taccsite_cms/settings.py, uncomment FAVICON setting.
  10. Restart server (if necessary).
  11. Verify logo is default — white "PORTAL".
  12. Verify favicon is default — white star on blue field.
  13. Re-test feat: TV3-181 add ptatax_assets Core-CMS-Custom#260.

UI

Standard Custom Deprecated
2 3 Standard 6 7 Custom 11 12 Deprecated

Notes

Used on client via TACC/Core-CMS-Custom#260.

{% with settings.FAVICON as favicon %}
<link rel="icon" href="{% static favicon.img_file_src %}" type="image/x-icon" />
{% with settings.TACC_FAVICON as favicon %}
<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. 😀

@wesleyboar
Copy link
Member Author

wesleyboar commented Jan 30, 2024

I will return with a PR that supports prefixes TACC_ and CORE_CMS_ (and/or something else we decide) [supports PORTAL_ prefix], so that the confusing TACC_LOGO and TACC_FAVICON do not propagate.

@wesleyboar wesleyboar merged commit 85bda39 into main Jan 30, 2024
@wesleyboar wesleyboar deleted the feat/TV3-181-add-ptdatax-resources branch January 30, 2024 18:49
@wesleyboar
Copy link
Member Author

wesleyboar commented Jan 30, 2024

See CMD-79 next.

wesleyboar added a commit to TACC/tup-ui that referenced this pull request Mar 5, 2024
wesleyboar added a commit to TACC/tup-ui that referenced this pull request Mar 6, 2024
* feat(tup-cms): Core-CMS v4.8.2

* feat: @tacc/core-styles v2.24.1 ie Table Redesign

* feat: @tacc/core-styles v2.25.1 revert Table Redesign

* feat: Core-CMS v4.8 new core-styles + a11y fixes

* feat: TACC/Core-CMS#808

* feat: TACC/Core-CMS#808 for real this time

* feat: TACC/Core-CMS#v4.8.3

* fix: deprecated FAVICON code (TACC/Core-CMS#783)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants