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

Use portal_url to root all CSS and rules file links #341

Open
wants to merge 2 commits into
base: 3.0.x
Choose a base branch
from

Conversation

erral
Copy link
Member

@erral erral commented Jul 19, 2023

No description provided.

@jensens
Copy link
Member

jensens commented Jul 19, 2023

  1. What is the problem solved here?
  2. Does this work together with collective.lineage and/or lineage.themeselection? The relative .. at least was very pleasant in such environments.
  3. Do we want this change in an old maintenance branch

@erral
Copy link
Member Author

erral commented Nov 22, 2023 via email

@yurj yurj self-requested a review July 23, 2024 14:14
Copy link
Contributor

@yurj yurj left a comment

Choose a reason for hiding this comment

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

LGTM

@yurj
Copy link
Contributor

yurj commented Jul 23, 2024

  1. What is the problem solved here?
  • different url for the same css file, cache problems
  • traversing private content can lead to "Insufficient privileges" while the resource itself is public
  1. Does this work together with collective.lineage and/or lineage.themeselection? The relative .. at least was very pleasant in such environments.

This should be solved in that packages, eventually. But we can avoid it implementing it just in the backend and let people use it in their own custom theme. backend.xml is part of this package but has the problems above (different url and traversing private content).

  1. Do we want this change in an old maintenance branch

I've created this PR on current master:
#376

@mtrebron
Copy link

I woud feel more comfortable if plain TAL would be used in the manifest and making this the canonical way. See other usage here: https://github.com/search?q=org%3Aplone+portal_state%2Fportal_url&type=code

@jensens
Copy link
Member

jensens commented Aug 8, 2024

I woud feel more comfortable if plain TAL would be used in the manifest and making this the canonical way. See other usage here: https://github.com/search?q=org%3Aplone+portal_state%2Fportal_url&type=code

Why? The usage here is IMO totally fine. And it is fast too.

@mtrebron
Copy link

mtrebron commented Aug 10, 2024

(edited) @jensens -#376 addresses this issue for manifest.cfg and was merged, I suggest we close this one. I have no opinion on the other parts of this PR.

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.

5 participants