marzer / poxy

Documentation generator for C++
https://pypi.org/project/poxy
MIT License
137 stars 5 forks source link

Remove excessive padding and text in github icon #5

Closed wroyca closed 2 years ago

wroyca commented 2 years ago

see: https://github.com/marzer/poxy/pull/5#issuecomment-1236397268

~The padding is excessively large for article sections. Since I'm not sure if this was intended or if it's a slight shape defect, I thought it best to open this up for investigation:~

(main) margin-top: 4rem (patch) margin-top: 2rem
marzer commented 2 years ago

Yep, it's intentional. In the default m.css style the "summary" sections tend to perceptually blend together somewhat when they get to a non-trivial length (i.e. many more members than in your example).

I played around with other solutions to it but ultimately increasing the padding to the current value was about the only one I was happy with.

It is a matter of taste/reading habits, of course. What to me gives the document some nice "breathing space", to others might be too sparse for comfort, so I'm not opposed to reducing it. 2rem looks like it will still be too small for my taste, but 3rem might be a good compromise.

wroyca commented 2 years ago

@marzer I saw that Poxy allows jquery scripting. While this goes against the css-only philosophy, maybe a tiny "compact mode" checkbox at the bottom of the table of contents would be sweet? Although I am not familiar with CSS, it may even be possible to use pure-css with something akin to:

https://www.w3schools.com/cssref/sel_gen_sibling.asp

#foo:checked ~ bar {
  merging-top: 1rem;
}
marzer commented 2 years ago

While this goes against the css-only philosophy

It's opt-in. It's just a "there if you want it" feature. I'm generally not as attached to the no-JS opinion held by m.css so I don't mind a small amount of it, within reason (I don't want 100 frameworks and a bunch of 'modern web' crap though, so on that note mosra and I are in firm agreement :D)

(Also the built-in search provided by m.css is powered by JS)

Although I am not familiar with CSS, it may even be possible to [...]

Maybe? I'm not that familiar with CSS either, to be honest. Most/all of my CSS knowledge is about 10 years old at this point. That being said, how would that specific solution be any different? If the net effect is visual space between consecutive sections being made larger or smaller, does it matter if it's done using margins or internal padding? Guess I'm unclear on the specific aspect you're actually wanting to change - is it purely visual or some other semantics?

maybe a tiny "compact mode" checkbox at the bottom of the table of contents would be sweet

This is a great idea. My go-to way of doing that would have been site-wide (binding the checkbox state to a cookie), but I have no idea what the state-of-the-art in webdev is these days. Likely 400 javascript frameworks and server-side rendering, lol. There's also the question of where the option toggle should go; under the menu seems like a logical choice but the floating menus can get pretty long so there's always a chance that it'd get pushed off the page. Pretty easy to prototype there and move it later, though, so I guess that's moot.

I'm actually about to go on a month-long overseas holiday in about a week's time and won't really be doing any tech work while I'm away; Any chance you want to do some prototyping of it in that time? I'll be unable to do so myself until around the 20th of October.

wroyca commented 2 years ago

Any chance you want to do some prototyping of it in that time? I'll be unable to do so myself until around the 20th of October.

I'm going to attempt a prototype this week or so. It's something I've been meaning to do locally for my own use, so it fits into my schedule. In the meantime, I'll commit the prototype to this RP so you can dive in easily as you get back from vacation

Guess I'm unclear on the specific aspect you're actually wanting to change - is it purely visual or some other semantics?

From what I've seen, CSS allows the child to "inherit" the value of the checked selector. This may allow poxy to not use JS for toggling http://jsfiddle.net/8b9j1ex5/6/ but this is something I am not familiar with, so I don't know how complex it would be to integrate it into Doxygen html generation

(Also the built-in search provided by m.css is powered by JS)

Now I understand why its so great. :)

marzer commented 2 years ago

I'll commit the prototype to this RP so you can dive in easily as you get back from vacation

Awesome! Looking forward to seeing what you come up with.

Now I understand why its so great. :)

Mosra did an in-depth write-up on it when it was first implemented, check it out if you're interested in that sort of thing: https://blog.magnum.graphics/meta/implementing-a-fast-doxygen-search/

mosra commented 2 years ago

Hmm, what if m.css had CSS theme "option" for the between-section padding? I vaguely remember I had some TODO item for this, but never got to actually finishing that...

As in, I have margin-bottom: 1rem; hardcoded on many places in the CSS and it might make more sense to have that configurable via a variable in the theme file instead, which you could then adapt to your liking. Plus instead of all margin being the same value, it could be different in usual text flow vs. between sections, for example. And it could benefit both of you I think, and shrink the set of patches poxy needs to apply on my CSS.

Though I can't say when I would be able to implement that. Constantly juggling 2-3 long-term contracts at the same time, and m.css is not among those unfortunately. A PR welcome though, I can provide a guidance -- as always :)

marzer commented 2 years ago

@mosra Interesting - so the 'compiled' themes are essentually manually-inlined and preprocessed versions of the original inputs. My understanding of CSS is fairly limited, but if the process was tweaked to inline the 'imports' without manually substituting the variables, and the theme variable files were distributed along with the rest of the docs HTML, then that should 'still work', right? And allow for some dynamic switching of the theme from JS or somesuch?

Or more simply, for m-dark.css, does this make sense:

/* omit this altogether and instead handle it in JS/HTML for dynamic shenanigans  */
/* @import url('m-theme-dark.css'); */

/* inline these imports but leave the var(...) expressions as-is */
@import url('m-grid.css');
@import url('m-components.css');
@import url('m-layout.css');
@import url('pygments-dark.css'); /* ...this one seems problematic */
@import url('pygments-console.css');

I guess one caveat would be that the browsers need to support CSS variables, whereas with preprocessed var() expressions that wouldn't be an issue. (caniuse.com seems to think that's largely a non-issue).

mosra commented 2 years ago

so the 'compiled' themes are essentually manually-inlined and preprocessed versions of the original inputs.

Yes, although not manually, there's a script for that :)

but if the process was tweaked to inline the 'imports'

No need to tweak anything -- if you include m-dark.css via <link rel="stylesheet">, instead of m-dark.compiled.css or whatever, and provide the original sources on the server too, the browsers will understand that. It's compiled because it's just a single HTTP request that way, and as the variables are always way longer than the value that's substituted, it's also much smaller and better compressible that way. (It was originally there for IE8 compatibility, but five years later it's not an issue).

But yes, this way you could switch the CSS on-the-fly without doing nasty JS stuff. Ages ago I did a similar thing for a fan website for the Archer series (click on the color swatches at the top), all it involved was setting a disabled attributed on a particular <link>. Well, and then a cookie to remember that for next visit. Which means, EU cookie banners, and popups, and opt-in buttons and ... isn't a single style actually bearable compared to that? :laughing:

marzer commented 2 years ago

Yes, although not manually

Heh, I meant manually as in 'you manually wrote a bespoke tool for performing that task', not that you were literally doing it by hand like a psycho. Poor choice of words I guess :D

No need to tweak anything -- if you include m-dark.css [...]

Well. This gives me ideas. I am annoyed that I'm going on vacation now.

mosra commented 2 years ago

Here's some actual docs on the feature I linked above: https://developer.mozilla.org/en-US/docs/Web/CSS/Alternative_style_sheets

(I think?) it's the same technology as what makes dark/light mode switching. Funnily enough, there's a link to an example on that page, claiming that Firefox supports even browser-side switching, but I couldn't find the menu item it refers to anywhere in the dumbed-down UI...

wroyca commented 2 years ago

@marzer Here is a showcase of what I experimented with today. Instead of adding the toggle button in the table of contents, I added a simple filter icon in the navigation bar, next to the search and github svg. I took the liberty of transforming the github icon to be more in line with the search icon, I hope you don't mind. This is purely done in CSS so the integration doesn't clash with m.css "js-free" philosophy. I'll try to push through it later tonight, but I probably won't have time to do so until tomorrow.

@mosra Let me know what you think too, and if this could be integrated into m.css along with your proposal to integrate a theme "option" for padding. I wouldn't mind giving it a shot this month.

Granted, this is just a video for now, I still have some cleaning and tweaking to do before I can commit. Regardless, it's a nice overview.

Note: The hover and active effect are currently missing, I ran out of time for this, but they will be added before I commit.

Edit: This is a toggle in case the video isn't clear enough. Unfortunately the screencast software is really terrible and I had to do several takes hoping the frames wouldn't get lost in the recording, hence why I didn't scroll through the document in the video. :/

Screencast from 2022-09-05 15-17-40.webm

marzer commented 2 years ago

@wroyca Looks great! I like the github button change - that's probably how it should have always been.

I'm currently in the process of tweaking the way CSS files are included to use the un-pre-processed @import versions w/ variables. I won't publish a new release from it just yet but will push it so you can at least see what I've done and/or experiment with it.

marzer commented 2 years ago

@wroyca I've pushed those changes to main. The CSS file layout now mimmicks the m.css one, in that there's two 'entry' css files, with only one ever being included depending on the theme:

Should make it easy to implement any dynamic theme switching shenanigans, as well as providing a place to 'inject' poxy variable overrides without needing to do clunky crap with !important. Eventually I'll try to backport the overrides I've already made to this new layout since it's clearly a much better way of doing it, heh.

I don't think this will impact your PR all that much, other than maybe making it easier to override stuff at the 'right' level.

wroyca commented 2 years ago

@wroyca I've pushed those changes to main. The CSS file layout now mimmicks the m.css one, in that there's two 'entry' css files, with only one ever being included depending on the theme:

Should make it easy to implement any dynamic theme switching shenanigans, as well as providing a place to 'inject' poxy variable overrides without needing to do clunky crap with !important. Eventually I'll try to backport the overrides I've already made to this new layout since it's clearly a much better way of doing it, heh.

I don't think this will impact your PR all that much, other than maybe making it easier to override stuff at the 'right' level.

Thank! I'll take a look and make adjustments if necessary

mosra commented 2 years ago

You might still want to apply the processing script nevertheless, to avoid too deep dependent HTTP requests ;) Especially the @import statements mean the browser will first have to wait until the top-level CSS is downloaded and only then it can submit a request for the remaining CSS files.

The css/postprocess.py script should be hopefully easy enough to import and call into from within poxy itself -- thus possible to use even with CSS overrides done by the user. Didn't see it being done in https://github.com/marzer/poxy/commit/c90fa961722688be579f96d5cfb0a48554e95be0, apologies if I missed that.

marzer commented 2 years ago

Didn't see it being done in https://github.com/marzer/poxy/commit/c90fa961722688be579f96d5cfb0a48554e95be0, apologies if I missed that.

Nah nothing like that is implemented yet. On the to-do list, though.

marzer commented 2 years ago

@mosra on the topic of reducing HTTP requests: I noticed that the google fonts do a fairly large number of fetches in their own right. Have you ever experienced this to be an issue? I am mulling over an idea of preprocessing those and putting their payloads in the output folder directly (modulo concerns about them going stale)

wroyca commented 2 years ago

@mosra on the topic of reducing HTTP requests: I noticed that the google fonts do a fairly large number of fetches in their own right. Have you ever experienced this to be an issue? I am mulling over an idea of preprocessing those and putting their payloads in the output folder directly (modulo concerns about them going stale)

On the subject of fonts: they were previously broken for the light theme, I didn't have time to check if they are still broken with the latest changes

marzer commented 2 years ago

@wroyca If they were before, they shouldn't be now. There was a bit of dumb special-handling that I removed so now dark + light are handled in exactly the same manner.

mosra commented 2 years ago

Re Google Fonts, see also https://github.com/mosra/m.css/issues/152. This is one of the larger topics I want to tackle, to not have a dependency on a 3rd party service. Because for me at least, if the pages load slow, it's due to these large CDNs being flaky, not due to my server being overloaded.

marzer commented 2 years ago

@mosra Ah, excellent, thanks. I was half expecting you to tell me either "it's never been a problem" or "it's a bad idea for reasons X, Y, Z", but it seems as though my instincts were vaguely sane (particularly on account of the browser cache partitioning discussed in that linked issue, which is a TIL for me).

Also the referrerpolicy thing is a good idea too.

marzer commented 2 years ago

@wroyca RE the original reason for opening this issue: on a whim I just changed the padding constant back to the 1rem default and it looks fine; I'm wondering if I did that before making some other padding-related change and now they've compounded together without me noticing.

So on that note I'd happily accept this pr as-is (or even: delete that CSS block altogether and use the m.css default)

wroyca commented 2 years ago

@wroyca RE the original reason for opening this issue: on a whim I just changed the padding constant back to the 1rem default and it looks fine; I'm wondering if I did that before making some other padding-related change and now they've compounded together without me noticing.

So on that note I'd happily accept this pr as-is (or even: delete that CSS block altogether and use the m.css default)

That's fine for me, I'll rebase and dissect the GitHub logo

wroyca commented 2 years ago

@marzer It's ready now. I noticed too late that you deleted the svg on c90fa96. I'm not sure what you intend to do here, so I'll leave it to you if that's okay? https://github.com/marzer/poxy/blob/c90fa961722688be579f96d5cfb0a48554e95be0/poxy/data/poxy.css#L81-L88

marzer commented 2 years ago

I noticed too late that you deleted the svg [...]

I moved the image url to the theme-specific file. Previously poxy would select the theme file + github icon file and rename them to not have the theme as part of their name, but I removed that behaviour since it doesn't make much sense if I eventually add in some functionality for dynamic switching between light+ dark.

tl,dr: your changes should be fine, but you don't need to have the image url there since it will be overridden anyway.

marzer commented 2 years ago

Thanks @wroyca!