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

Initialize head theme resources in combined AJAX responses. #3635

Conversation

demiankatz
Copy link
Member

The code to generate combined search AJAX responses was intended to output CSS links in case of dynamic style addition during template rendering. However, this logic did not quite work correctly, and would sometimes cause default styles to override custom styles. This PR properly initializes resources and changes order of operations to ensure that template rendering is completed before style links are added to the response.

@demiankatz demiankatz added this to the 9.1.2 milestone May 1, 2024
@maccabeelevine
Copy link
Member

However, this logic did not quite work correctly, and would sometimes cause default styles to override custom styles.

I see how the PR changes the order of operations as you're intending, but I'm having trouble reproducing the original problem -- CSS changes I'm making in an included module are consistently overwriting the global stuff. Any suggestion?

@demiankatz
Copy link
Member Author

@maccabeelevine, the specific problem that was occurring before was that the combined search columns were ONLY getting headed with compiled.css; no other files were being included. Most of the time, this was not a problem, but if you had styles in a local CSS file that overrode styles in compiled.css, the subsequent reload of compiled.css would make it take precedence over the local custom styles and cancel them out.

While I think the current order of operations makes more logical sense than the previous order of operations, I confess that I don't fully understand what was causing the previous incorrect behavior. It didn't make a great deal of sense to me. I actually think the BEST solution would be if we could have the combined search handler output ONLY files that are added dynamically during template rendering, and not the ones that are included in theme.config.php (since those are already loaded by the containing page anyway). However, I couldn't figure out a way to do that without further overriding the Laminas helper behavior, and since we already are going to have a project ahead of us to revise our Laminas-derived helpers due to major version changes, I wasn't especially interested in making things even more complicated at this point in time. This solution seems suboptimal but at least potentially acceptable. I'd definitely welcome better ideas if you have any, though!

@maccabeelevine
Copy link
Member

@maccabeelevine, the specific problem that was occurring before was that the combined search columns were ONLY getting headed with compiled.css; no other files were being included. Most of the time, this was not a problem, but if you had styles in a local CSS file that overrode styles in compiled.css, the subsequent reload of compiled.css would make it take precedence over the local custom styles and cancel them out.

@demiankatz Sorry I must be dense here. On dev branch I have a combined search with Solr and EDS boxes. EDS result-list.phtml is loading EDS.css at the top. It works fine and declares it after compiled.css.

image

So I have a dummy style in combined.css that I'm overriding in EDS.css, and EDS wins. What am I missing?

@demiankatz
Copy link
Member Author

demiankatz commented May 8, 2024

Here's how I set up a test to reproduce the problem:

1.) I edited themes/sandal/theme.config.php to add 'css' => ['custom.css'], at the top of the config array.

2.) I added themes/sandal/css/custom.css:

body {
  color: green;
}

3.) I set up a combined search with at least one AJAX column (doesn't matter what targets you use).

In this situation, when the combined search results page loads, the text will be green until the AJAX columns load in, and then it will turn black. If you turn off ajax in combined.ini, the text will stay green.

Copy link
Member

@maccabeelevine maccabeelevine left a comment

Choose a reason for hiding this comment

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

@demiankatz Ok thank you, when you said "custom styles" in the context of combined search, I thought you meant styles loaded by the specific combined search modules. But of course you meant actual custom (local) CSS. So yes, problem reproduced and fixed as you suggest, the custom.css is now loaded last.

My misunderstanding shows something else interesting -- the actual effective order is now

  1. compiled.css
  2. custom.css
  3. module-specific css

So if I set a body text color in both custom.css and (say) EDS.css, the one in EDS still wins. FWIW that's true even if I add the custom.css in sandal theme and edit the EDS.css in its parent bootstrap3 theme. And true regardless of ajax mode. But I don't know if that is actually a problem, and even if so It should be handled separately.

@demiankatz
Copy link
Member Author

Thanks, @maccabeelevine -- what you describe makes sense based on the order the files get loaded. CSS files from theme.config.php are always going to get loaded ahead of CSS files dynamically loaded in templates. As you say, that could potentially cause problems for overriding certain styles and lead to unintuitive behavior. It might be necessary in some circumstances to override entire files (e.g. an EDS.css in your local theme that imports the parent version from the other theme using a relative path and then overrides some pieces), use more specific selectors, etc. So far, no one has complained about it, so I don't think there's a strong need to fix it. But it's a good thing to be aware of in case it comes up in future!

@demiankatz demiankatz merged commit d511af4 into vufind-org:dev May 8, 2024
7 checks passed
@demiankatz demiankatz deleted the release-9.1-init-headthemeresources-in-combined branch May 8, 2024 20:23
@demiankatz
Copy link
Member Author

Oops -- merged this to dev instead of release-9.1 by accident; I've cherry-picked it backwards to compensate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants