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

NG: Theme modernization, last inches #510

Merged
merged 14 commits into from
Aug 7, 2024

Conversation

msbt
Copy link
Collaborator

@msbt msbt commented Jul 19, 2024

About

The first bunch of ng-fixes. Mostly taking care about styling anomalies reported by others.

  • Fonts.
  • Admonitions.
  • Paragraphs in <ul> elements.

Preview

https://crate-docs-theme--510.org.readthedocs.build/en/510/

@amotl amotl changed the base branch from main to amo/basic-ng July 19, 2024 14:43
@amotl
Copy link
Member

amotl commented Jul 20, 2024

Thank you very much. We've just released version 0.34.0.dev2, including your improvements.

@amotl

This comment was marked as duplicate.

Comment on lines 1838 to 1631
/* remove margin from paragraphs in ul & ol li's */
main ul li p,
main ol li p {
margin: 0;
}
Copy link
Member

Choose a reason for hiding this comment

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

Hi. Thanks a stack for those improvements. In my other patch crate/cratedb-guide#53, I was running such inline CSS directives, apparently also adjust some undesired rendering behavior.

<style>
.field-list dd {
  margin-bottom: 1em !important;
}
.field-list p {
  margin-bottom: 0.5em;
}
</style>

Do you think they are related to your improvements, made obsolete by them, and could potentially be removed from the inline spots of that patch, when using the newest and shiniest of our modernizations?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@amotl not as of now, since we specifically target ul and ol's, not dl's, so none of those would be targeted by the other css.

Comment on lines 81 to 90
h2 {
font-size: 30px;
line-height: 36px; /* this heading font-size * 1.2 */
margin-top: 0px; /* this heading font-size */
margin-top: -18px;
margin-bottom: 15px; /* this heading font-size * 0.5 */
padding-top: 30px;
}

h3 {
font-size: 27px; /* round(previous heading font-size * 0.9) */
font-size: 26px; /* round(previous heading font-size * 0.9) */
line-height: 32px; /* round( this heading font-size * 1.2) */
margin-top: 0px; /* this heading font-size */
margin-top: -14px;
margin-bottom: 14px; /* round( this heading font-size * 0.5) */
padding-top: 27px;
}

h4 {
font-size: 24px; /* round(previous heading font-size * 0.9) */
line-height: 29px; /* round( this heading font-size * 1.2) */
margin-top: 0; /* this heading font-size */
margin-top: -12px;
margin-bottom: 12px; /* round( this heading font-size * 0.5) */
padding-top: 24px;
}

h5 {
font-size: 22px; /* round(previous heading font-size * 0.9) */
line-height: 26px; /* round( this heading font-size * 1.2) */
margin-top: 0; /* this heading font-size */
margin-bottom: 11px; /* round( this heading font-size * 0.5) */
padding-top: 22px;
}

h5 {
font-size: 20px; /* round(previous heading font-size * 0.9) */
line-height: 24px; /* round( this heading font-size * 1.2) */
margin-top: 0; /* this heading font-size */
margin-top: -10px;
margin-bottom: 10px; /* round( this heading font-size * 0.5) */
padding-top: 20px;
}

h6 {
font-size: 18px; /* round(previous heading font-size * 0.9) */
line-height: 22px; /* round( this heading font-size * 1.2) */
margin-top: 0; /* this heading font-size */
margin-top: -9px;
margin-bottom: 9px; /* round( this heading font-size * 0.5) */
padding-top: 18px;
}
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be applicable to just remove all custom style directives on the <h[1-6]> elements? Is there any reason to adjust them at all? What would happen otherwise?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@amotl since we don't import the fontstuff from furo yet, the only fallback we have is the h1 of normalize.css, everything else would fall back to the browsers default sizes which doesn't look great. Howevery, this part needs one more round of finetuning, the scrollpositions aren't quite right. Or do you want to try and use furos typography styles?

@amotl
Copy link
Member

amotl commented Jul 20, 2024

Another request for this fixup patch: Please compare the margins / paddings of the <li> item paragraphs between vanilla Furo and our theme.

In our theme, something seems to be configured too tight in this area. Furo offers a much better appearance and reading flow. Can we improve that? Maybe it's all just my fault by adding those custom styles to the CrateDB Guide project?

@msbt
Copy link
Collaborator Author

msbt commented Jul 22, 2024

In our theme, something seems to be configured too tight in this area. Furo offers a much better appearance and reading flow. Can we improve that? Maybe it's all just my fault by adding those custom styles to the CrateDB Guide project?

Most of those styles aren't applied since we don't have that .wrapper-content-right container anymore. I might have been too strict with the margins the last time, I took some inspiration of the furo styles and applied them to our theme, need to re-check all critical pages though ;)

@amotl
Copy link
Member

amotl commented Jul 22, 2024

Hi again. I would like to report when the promotion header is displayed, it hides the headline. Can you adjust the stickyness behaviour in that area? Example: https://cratedb.com/docs/guide/feature/

image

I guess it is difficult to spot in preview mode, because on the RTD preview page, there is a another custom header inlined, which shifts the headline towards the bottom, that's why it is still visible. On a local workstation, it is also difficult, because it doesn't do any ESI, so it doesn't display any promotion header at all. Right?

@amotl amotl changed the title Admonition and font/paragraph styling NG: Theme modernization, last inches Jul 22, 2024
@msbt
Copy link
Collaborator Author

msbt commented Jul 24, 2024

@amotl correct, the promo header ESI is only triggered when served via reverse proxy (i.e. on a live-domain like cratedb.com). I made a few fixes in the last commit which removes a lot of the logic that we were using and made the banner non-sticky and positioned the other elements relative to it, so it shouldn't overlap anything and hide when scrolling down.

@amotl
Copy link
Member

amotl commented Jul 24, 2024

Thanks. I will run a another devX release later.

@amotl
Copy link
Member

amotl commented Jul 31, 2024

@proddata mentioned that something is not ok with table renderings? Was it different on dev3, and what's the problem? Missing border lines?

image

Georg:
Yes, missing border lines and quite a bit more spacing

image

@amotl
Copy link
Member

amotl commented Jul 31, 2024

Yes, missing border lines and quite a bit more spacing.

Thanks. Let's wait for another iteration then. @msbt: Let's talk about it tomorrow?

Before: https://cratedb.com/docs/cloud/en/latest/reference/services.html#shared
After: https://crate-cloud--79.org.readthedocs.build/en/79/reference/services.html#shared

@amotl
Copy link
Member

amotl commented Aug 5, 2024

@proddata mentioned that something is not ok with table renderings?

@msbt fixed it. Thanks!

image

See https://crate-cloud--79.org.readthedocs.build/en/79/reference/services.html#dedicated.

@amotl
Copy link
Member

amotl commented Aug 5, 2024

Vertical spacing between <li> link elements, for example on rendered page tocs, is also much better now. Thanks, @msbt!

Before / After, on https://crate--16344.org.readthedocs.build/en/16344/concepts/clustering.html.

image image

@amotl
Copy link
Member

amotl commented Aug 5, 2024

@matkuliak, @proddata, and @matriv / @mkleen / @BaurzhanSakhariev / @surister: If you don't see any other flaws that would block the release of the revamped documentation theme, we may be able to release it before heading into the summer breaks.

All of those use 0.34.0.dev6 now, based on recent improvements from this PR, and also includes changes from GH-515.

/cc @mfussenegger, @romseygeek, @simonprickett, @widmogrod

@proddata
Copy link
Member

proddata commented Aug 6, 2024

If you don't see any other flaws that would block the release of the revamped documentation theme, we may be able to release it before heading into the summer breaks.

Fine for the CrateDB Cloud docs 👍
Smaller bugs can be fixed afterwards.

@amotl amotl force-pushed the matthias/first-bunch-of-ng-fixes branch from 8f3437c to c013bba Compare August 7, 2024 15:14
@amotl amotl marked this pull request as ready for review August 7, 2024 15:16
Copy link
Member

@amotl amotl left a comment

Choose a reason for hiding this comment

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

@msbt: Thanks a stack for all the aftermath fixes to GH-390. Your patch will be merged and included into the upcoming release.

@amotl amotl merged commit 3764f89 into amo/basic-ng Aug 7, 2024
8 checks passed
@amotl amotl deleted the matthias/first-bunch-of-ng-fixes branch August 7, 2024 15:18
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