-
Notifications
You must be signed in to change notification settings - Fork 288
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
Update css & js #1407
base: master
Are you sure you want to change the base?
Update css & js #1407
Conversation
Build successful. You can preview it here: https://preview.aclanthology.org/update-css-js |
This commit also fixes aria labels for the search box; forgot to mention that. Together with a better caching policy for our assets and enabling http/2, this should make the site faster for slow connections. It is also the first step to proper minification of css and js. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! But should we really be bumping the major version of the Bootstrap JS when our Bootstrap CSS is using 4.x? I see you changed some HTML attributes to adapt, but that means that now there's a mismatch between which Bootstrap documentation to look at for modifying the CSS versus the JS. Also not sure if it might cause things to break (though in the preview branch I couldn't see any problems).
I'm ambivalent about not using the FontAwesome CDN anymore (we discussed this before) but it's not a hill I will die on. :)
Good point regarding bootstrap CSS -- I will bump that as well. Re FontAwesomeCDN: The problem is as follows: fa is a css and a font (.woff2) resource. When we use a CDN, we need to load both from the CDN. This means:
Now:
With http/2, we only need a single connection in contrast to several and it does speed things up according to my measurements. Maybe we could also preload from the CDN, but then we rely on links to the fonts never changing in the externally hosted css. When we serve fa ourselves, we can minify it and only serve the subset the anthology is actually using. |
Note that bumping the Bootstrap CSS is a bit more involved, and would probably require careful checking of the migration guide here: https://getbootstrap.com/docs/5.0/migration/ |
Counterpoint would be that our CSS gets bigger, and the font always needs to be requested instead of potentially being reused from cache if the user already visited another site using FontAwesome before. |
I'll leave this one to you two to resolve. |
I made it a draft and will push this and that in the near future. |
It might also be a good time to update Hugo, if it's not too much extra work. Our version is getting a bit dated. We could make this a general housekeeping update. |
I already snuck in the relevant changes into this PR and use 0.85 locally :-) |
@akoehn any interest in updating this and pushing it through? I’ll do my best to do a fast turnaround on a review. |
Yes, I will continue to work on it soon-ish. Time to dust off my js&css skills from ~2004-2006 lol |
- bootstrap class names needed update - bundling css: we server local css anyway, so bundle external as well and save some roundtrips
I rebased and force-pushed this branch because I did not want to merge one year of work. This is still WIP, as a lot of CSS has changed (also, jquery was dropped) |
I'm going through the CSS changes for bootstrap 5 and the behavior of badges has changed. We are currently using badges in a lot of places where we provide a hyperlink (link to bibliography, code, PDF, ...) and bootstrap dropped support for hover on badges. I think they want badges to be non-clickable elements and push people to use buttons instead. I tried using buttons but these are too big for the list view: OTOH, the badges were quite small and maybe not that accessible. Thoughts? |
I don't see a good reason to change the styling; the badges are currently as high as the text that follows, so if people find them too small they'd probably enlarge the entire site anyway. But that's just my guess, I'm happy to hear of other's experiences. You could argue that semantically, these indeed act like buttons, which might make a difference from a semantic XML/screenreader etc. perspective. I don't have enough experience with that. If the current behavior (w.r.t. to hovering etc.) can only be replicated with buttons in BS5, my first thought would be to make a CSS class |
There are two possible approaches:
I think I prefer the first one because it is cleaner and also probably easier to implement (at least not harder). Interestingly, there was a |
1 for doing the first one. Seems to make more sense from a semantic perspective. |
I finally got around to continue this transition (and re-learning css on the way), there is one thing I am struggling with: |
I have pushed the current state. Known problems:
The last two are probably JS related, as bootstrap has dropped jquery; I have not started working on the js changes yet. |
I think that used to be generated from the "attachment" theme color that's added in I'm completely swamped with work at the moment so it'll be a while until I can take a closer look at the changes. |
Thanks @mbollmann that was the hint I needed :-) Everything should be fixed now, please have a look at the preview -- after it is built. one thing to note: bootstrap 5 has changed the font handling a bit and you might find that the new version uses different fonts (it does on my system). Bootstrap always tried to use system fonts by default and how it does that has evolved a bit. |
Thanks for picking this up! In Safari on a Mac, things do not look right:
|
Will look into underline & icon spacing (I have that as well, but here the lines are thinner so I did not notice ...) Is the second image a forced narrow rendering or what is the context? |
The new default font choice is ... surprising. On my system, it's remarkably different from how it used to look, and my first reaction is that I find it unpleasant: I also never really noticed this big of a difference between systems with the current site (though I haven't explicitly tested it). Also, I notice that the font color is now black on all the buttons, whereas before it used to be white on buttons with sufficiently dark background. I believe the old Bootstrap used to determine that automatically based on an SCSS function that determines color brightness or something. Does this need to be done explicitly now? |
Ah, so the new Bootstrap defaults to whatever the user's system UI font is. That means it's completely unpredictable what the font will be for a given user, which I'm not sure I'm a big fan of from a branding perspective... At the very least, it seems this would make it harder to test layout changes & ensure they look good for everyone? I haven't looked into Bootstrap's rationale for this change, though. |
The new default font should be closer to your system default, the old one you saw is still there as a fallback in case the correct one cannot be loaded. The rationale is unchanged, the method has just evolved. Edit: in other words: the font has not become less predictable; bootstrap 4 already tried to use the system font. If I remember / understand the theming for the buttons correctly, they have black text on everything brighter than 50% and white on darker than 50%. I noticed that as well last year, looked into it and they had a reason to change it but I forgot what it was. underline & icon spacing is now fixed. |
Thanks! I will look at the scaling issue. Can reproduce when zooming in to 150%. |
Links still underlined on list pages (volume/author pages). I still feel strongly that the font should be set to something more predictable. At the risk of sounding like a grumpy old Linux user, the font I set for my system UI is very much not a font I'd like to see websites or documents in, as they serve very different purposes. That, plus all the other reasons I gave in my last comment. Still happy to be convinced otherwise; maybe I'm the only weirdo who will complain that the Anthology suddenly got a lot uglier. |
We can use a custom font; all I wanted to say is that bootstrap 4 already tried to use a system font and the main difference for bootstrap 5 is that there is a new approach to find the system font, which may result in a different font being used now. There is no new feature or explicitly changed font here. Wrapping for the landing page is updated, the header is not looking correct and the link underline for lists needs a look (this underline thing is a mess) Do you have a font to propose? I tried computer modern for fun, but it is very dense. |
@akoehn sorry that this got ditched. Do you think it could be merged, or is it just too out of date? |
Oh, I see it's still a draft. I'm going to let the preview build, then take a look. |
See commit messages for contents; mainly housekeeping and cleaning up a bit