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

Update css & js #1407

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from
Draft

Update css & js #1407

wants to merge 16 commits into from

Conversation

akoehn
Copy link
Member

@akoehn akoehn commented Jul 8, 2021

See commit messages for contents; mainly housekeeping and cleaning up a bit

@github-actions
Copy link

github-actions bot commented Jul 8, 2021

Build successful. You can preview it here: https://preview.aclanthology.org/update-css-js
This preview will be removed when the branch is merged.

@akoehn
Copy link
Member Author

akoehn commented Jul 9, 2021

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.

Copy link
Member

@mbollmann mbollmann left a 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. :)

@akoehn
Copy link
Member Author

akoehn commented Jul 9, 2021

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:

  • load html
  • see that we need fa css, request that
  • once fa css is there, request the font
  • for both, we load everything even though we only use a small subset

Now:

  • load html, css&font marked as preload
  • css is merged with our main css, no overhead there
  • font is requested from our site

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.

@mbollmann
Copy link
Member

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/

@mbollmann
Copy link
Member

* css is merged with our main css, no overhead there

* font is requested from our site

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.

@mjpost
Copy link
Member

mjpost commented Jul 11, 2021

I'll leave this one to you two to resolve.

@akoehn akoehn marked this pull request as draft July 12, 2021 15:27
@akoehn
Copy link
Member Author

akoehn commented Jul 12, 2021

I made it a draft and will push this and that in the near future.

@mjpost
Copy link
Member

mjpost commented Jul 12, 2021

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.

@akoehn
Copy link
Member Author

akoehn commented Jul 13, 2021

It might also be a good time to update Hugo

I already snuck in the relevant changes into this PR and use 0.85 locally :-)

@mjpost
Copy link
Member

mjpost commented Feb 14, 2022

@akoehn any interest in updating this and pushing it through? I’ll do my best to do a fast turnaround on a review.

@akoehn
Copy link
Member Author

akoehn commented Feb 15, 2022

Yes, I will continue to work on it soon-ish. Time to dust off my js&css skills from ~2004-2006 lol

@akoehn
Copy link
Member Author

akoehn commented Mar 17, 2022

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)

@akoehn
Copy link
Member Author

akoehn commented Mar 22, 2022

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:

grafik

OTOH, the badges were quite small and maybe not that accessible. Thoughts?

@mbollmann
Copy link
Member

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 btn-badgelike or btn-tiny or something, to emulate the look of the old badges. I would start by copying the source for the btn-sm class, copy margin & padding values (and probably font size/style) from the old badges, and see where that gets us.

@akoehn
Copy link
Member Author

akoehn commented Mar 22, 2022

There are two possible approaches:

  • make tiny buttons
  • make "buttonized" badges (i.e. make them react to hover again)

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 btn-xs in BS4, but that, too, was dropped in BS5.

@mbollmann
Copy link
Member

+1 for doing the first one. Seems to make more sense from a semantic perspective.

@akoehn
Copy link
Member Author

akoehn commented Apr 20, 2022

I finally got around to continue this transition (and re-learning css on the way), there is one thing I am struggling with:
Some "buttons" (which partly used to be badges) have the class btn-attachment and badge-attachment, but I cannot find the definition of the classes. @mbollmann do you know this?

@akoehn
Copy link
Member Author

akoehn commented Apr 20, 2022

I have pushed the current state. Known problems:

  • attachment button not rendered correctly (see Q above)
  • "more options" in the copy citations line is not working
  • cite -> preformatted not working

The last two are probably JS related, as bootstrap has dropped jquery; I have not started working on the js changes yet.
Please have a look whether there is anything else not working as intended or not looking good.

@mbollmann
Copy link
Member

I finally got around to continue this transition (and re-learning css on the way), there is one thing I am struggling with: Some "buttons" (which partly used to be badges) have the class btn-attachment and badge-attachment, but I cannot find the definition of the classes. @mbollmann do you know this?

I think that used to be generated from the "attachment" theme color that's added in hugo/assets/css/main.scss.

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.

@github-actions
Copy link

Build successful. Some useful links:

This preview will be removed when the branch is merged.

@akoehn
Copy link
Member Author

akoehn commented Apr 20, 2022

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.

@mjpost
Copy link
Member

mjpost commented Apr 20, 2022

Thanks for picking this up! In Safari on a Mac, things do not look right:

image

  • Main page not displaying info box correctly. Also no margins in the header (but maybe that's fine?)

image

@akoehn
Copy link
Member Author

akoehn commented Apr 20, 2022

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?

@mbollmann
Copy link
Member

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:

grafik

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?

@mbollmann
Copy link
Member

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.

@akoehn
Copy link
Member Author

akoehn commented Apr 20, 2022

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.

@mjpost
Copy link
Member

mjpost commented Apr 20, 2022

On my desktop (MacBook Pro in Safari, all latest) I still see this on the main page:

image

unless I zoom way out:

image

@akoehn
Copy link
Member Author

akoehn commented Apr 21, 2022

Thanks! I will look at the scaling issue. Can reproduce when zooming in to 150%.

@mbollmann
Copy link
Member

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.

@akoehn
Copy link
Member Author

akoehn commented Apr 21, 2022

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.

@acl-org acl-org deleted a comment from github-actions bot Nov 22, 2022
@mjpost
Copy link
Member

mjpost commented Nov 22, 2022

@akoehn sorry that this got ditched. Do you think it could be merged, or is it just too out of date?

@mjpost
Copy link
Member

mjpost commented Nov 22, 2022

Oh, I see it's still a draft. I'm going to let the preview build, then take a look.

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.

3 participants