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

Support CSS URLs when using root_path #8128

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

JVickery-TBS
Copy link
Contributor

@JVickery-TBS JVickery-TBS commented Mar 20, 2024

fix(css): uris with root_path;

  • Support CSS urls when using ckan.root_path.

Proposed fixes:

The main css files use url values with relative paths, which results in them 404ing if someone has set up CKAN using a ckan.root_path.

I have added CSS variables in a root css element in the base.html template so we can call the url_for_static helper. And used the variables throughout the stylesheets.

Features:

  • includes tests covering changes
  • includes updated documentation
  • includes user-visible changes
  • includes API changes
  • includes bugfix for possible backport

Please [X] all the boxes above that apply

- Support CSS urls when using `ckan.root_path` and/or a reverse proxy.
- Added change log file.
- Moved new css variables into its own style tage and block.
- Conditioned the `url_for_static` on the `ckan.root_path` config.
@JVickery-TBS JVickery-TBS changed the title Support CSS URLs when Using root_path and a Reverse Proxy Support CSS URLs when using root_path Mar 20, 2024
- Added new css variables to sass scripts.
- Made template condition better.
- Use `body` to override `:root` css variables.
@wardi
Copy link
Contributor

wardi commented Apr 17, 2024

@smotornyuk you mentioned that the cssrewrite filter should be able to fix static urls. How does that work?

@JVickery-TBS
Copy link
Contributor Author

https://webassets.readthedocs.io/en/latest/builtin_filters.html#cssrewrite

Not sure where in the ckan code to put something like this?

@smotornyuk
Copy link
Member

It's already been added to main/main-rtl assets

Ideally, it must rewrite URLs automatically when the asset is assembled. I wonder if it rewrites the URL but does it in a wrong way, or is not applied at all. I'm sure this filter worked before for some files(and that's why it's added everywhere).

If possible, it's better to debug the problem with the filter and fix it, or register a custom filter that does something similar appropriately. @JVickery-TBS, your fix is good for this particular case, but we need something that will work for every extension that relies on relative paths. I'd say that auto-fixing with cssrewrite looks tidier, so let's try debugging it and finding the problem.

@JVickery-TBS
Copy link
Contributor Author

@smotornyuk from my understanding of reading https://webassets.readthedocs.io/en/latest/builtin_filters.html#cssrewrite, it rewrites based on the location of the output path. But if using CKAN's root_path with a reverse proxy, the root_path does not actually exist and webassets would not include the ckan.root_path in the rewrite.

The filter works fine as far as I can tell. Is it just that I have not set up ckan.root_path correctly? Does this directory actually have to exist on the server and all of the source files have to be in it?

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

3 participants