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

Remove excessive padding and text in github icon #5

Merged
merged 2 commits into from
Sep 6, 2022
Merged

Remove excessive padding and text in github icon #5

merged 2 commits into from
Sep 6, 2022

Conversation

wroyca
Copy link
Contributor

@wroyca wroyca commented Sep 3, 2022

see: #5 (comment)

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
Copy link
Owner

marzer commented Sep 4, 2022

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
Copy link
Contributor Author

wroyca commented Sep 4, 2022

@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
Copy link
Owner

marzer commented Sep 4, 2022

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
Copy link
Contributor Author

wroyca commented Sep 4, 2022

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

  • If you can, turning this PR into a draft PR would be great!

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 marzer marked this pull request as draft September 4, 2022 21:06
@marzer
Copy link
Owner

marzer commented Sep 4, 2022

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
Copy link

mosra commented Sep 5, 2022

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 :)

@wroyca wroyca changed the title Lower article section padding Proposal: Interactive toggle to compact sections, regardless of the project's fixed margin preference. Sep 5, 2022
@marzer
Copy link
Owner

marzer commented Sep 5, 2022

@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
Copy link

mosra commented Sep 5, 2022

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? 😆

@marzer
Copy link
Owner

marzer commented Sep 5, 2022

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
Copy link

mosra commented Sep 5, 2022

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
Copy link
Contributor Author

wroyca commented Sep 5, 2022

@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
Copy link
Owner

marzer commented Sep 5, 2022

@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
Copy link
Owner

marzer commented Sep 5, 2022

@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
Copy link
Contributor Author

wroyca commented Sep 5, 2022

@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
Copy link

mosra commented Sep 6, 2022

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 c90fa96, apologies if I missed that.

@marzer
Copy link
Owner

marzer commented Sep 6, 2022

Didn't see it being done in c90fa96, apologies if I missed that.

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

@marzer
Copy link
Owner

marzer commented Sep 6, 2022

@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
Copy link
Contributor Author

wroyca commented Sep 6, 2022

@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
Copy link
Owner

marzer commented Sep 6, 2022

@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
Copy link

mosra commented Sep 6, 2022

Re Google Fonts, see also mosra/m.css#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
Copy link
Owner

marzer commented Sep 6, 2022

@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
Copy link
Owner

marzer commented Sep 6, 2022

@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
Copy link
Contributor Author

wroyca commented Sep 6, 2022

@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

Signed-off-by: William Roy <wroy@proton.me>
@wroyca wroyca marked this pull request as ready for review September 6, 2022 15:06
@wroyca
Copy link
Contributor Author

wroyca commented Sep 6, 2022

@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?

poxy/poxy/data/poxy.css

Lines 81 to 88 in c90fa96

nav .github
{
padding-left: 44px !important;
background-repeat: no-repeat;
background-size: 25px 25px;
background-position: -30px center;
background-origin: content-box;
}

@marzer
Copy link
Owner

marzer commented Sep 6, 2022

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 marzer changed the title Proposal: Interactive toggle to compact sections, regardless of the project's fixed margin preference. Remove excessive padding and text in github icon Sep 6, 2022
@marzer marzer merged commit a070af8 into marzer:main Sep 6, 2022
@marzer
Copy link
Owner

marzer commented Sep 6, 2022

Thanks @wroyca!

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