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

Better distinguish reference titles from subtitles #2596

Closed
wants to merge 1 commit into from

Conversation

salim-b
Copy link
Collaborator

@salim-b salim-b commented May 29, 2024

Minimalistic styling addition that underlines top-level reference topics ("titles") to better distinguish them from second-level ones ("subtitles"). I found it always hard to visually tell them apart with pkgdown's default styling, so I think this would make sense to be included by default.

@hadley
Copy link
Member

hadley commented May 29, 2024

Can you provide an example of the problem? I'm happy to improve the contrast, but I don't think underlines are the choice.

@salim-b
Copy link
Collaborator Author

salim-b commented May 29, 2024

Sure. Not the best example, but one:

default styling styling with this PR
Screenshot 2024-05-29 at 20-57-30 Package index Screenshot 2024-05-29 at 20-58-42 Package index

pkgdown reference index that produces the above:

reference:
- title: Common
  desc: Common functions that are of generic use.
- contents: api
- title: Meta
  desc: Functions to work with NocoDB's RESTful [meta APIs](https://meta-apis-v2.nocodb.com/).
- subtitle: Bases
  desc: Functions to manage NocoDB [bases](https://docs.nocodb.com/category/bases)
    (aka "projects").
- contents:
  - bases
  - base_id
  - base
  - create_base
  - update_base
- subtitle: Tables
  desc: Functions to manage NocoDB [tables](https://docs.nocodb.com/category/tables).
- contents:
  - tbls
  - tbl_id
  - tbl
  - update_tbl
  - reorder_tbl
  - set_tbl_metadata
- subtitle: Table columns
  desc: Functions to manage NocoDB table [columns](https://docs.nocodb.com/category/fields)
    (aka "fields").
- contents:
  - tbl_cols
  - tbl_col_id
  - tbl_col
  - update_tbl_col
  - set_display_val
  - set_display_vals
  - create_user

@salim-b
Copy link
Collaborator Author

salim-b commented May 29, 2024

I'm happy to improve the contrast, but I don't think underlines are the choice.

If you got a more compellling solution, even better! ☺️

@hadley
Copy link
Member

hadley commented May 29, 2024

Hmmm, I think the source of the problem might be bootstraps responsive font sizing — if you make the page wide (like I'm usually looking at), I think there's a decent distinction between h2 and h3. But when you make it narrow the distinction disappears.

@hadley
Copy link
Member

hadley commented May 29, 2024

As a starter for discussion, I'd propose:

h2, .h2 { font-size: 1.85rem;}
h3, .h3 { font-size: 1.5rem;}
h4, .h4 { font-size: 1.3rem}
h5, .h5 { font-size: 1.1rem}
// h6 is font-size: 1rem

@salim-b
Copy link
Collaborator Author

salim-b commented May 29, 2024

Hmmm, I think the source of the problem might be bootstraps responsive font sizing — if you make the page wide (like I'm usually looking at), I think there's a decent distinction between h2 and h3. But when you make it narrow the distinction disappears.

Nope, not really.

Bildschirmaufzeichnung.vom.29.05.2024.23.53.58.webm

@salim-b
Copy link
Collaborator Author

salim-b commented May 29, 2024

As a starter for discussion, I'd propose:

h2, .h2 { font-size: 1.85rem;}
h3, .h3 { font-size: 1.5rem;}
h4, .h4 { font-size: 1.3rem}
h5, .h5 { font-size: 1.1rem}
// h6 is font-size: 1rem

Current styling is

h2 { font-size: 2rem;}
h3 { font-size: 1.75rem;}
h4 { font-size: 1.5rem}
h5 { font-size: 1.25rem}
// h6 is font-size: 1rem

which does look fine on its own:

Screenshot 2024-05-30 at 00-10-55 Use NocoDB's RESTful APIs

It's just the reference page where I don't think the <h2> vs. <h3> distinction is clear enough. To the reader it should be clearly "visually conveyed" that an <h2> groups together everything that follows below it – incl. <h3>'s – up to the next <h2>.

For this, I still think the underlining is a pragmatic solution. Messing with the heading font sizes OTOH won't help too much here, I fear. But feel free to close this if you disagree. 🙂

@hadley
Copy link
Member

hadley commented May 30, 2024

Hmmm, maybe the problem is that the spacing between the individual entries is a bit large, making them not feel like a cohesive block. What do you think of dl {margin-bottom: 0.};?

@hadley
Copy link
Member

hadley commented May 30, 2024

Then that makes the heading margins a bit small, so I played around a bit and came up with this:

dl {margin-bottom: 0rem;}
h2, .h2 { font-size: 1.75rem; margin-top: 1.5rem;}
h3, .h3 { font-size: 1.2rem; margin-top: 0.5rem;}
h4, .h4 { font-size: 1.1rem}
h5, .h5 { font-size: 1rem}

I think this looks quite good on the articles too. (The downside is that there's no distinction between h5 and h6, but I think that's unlikely to matter much in practice)

(This is similar, if less dramatic, than what I did in tidytemplate: https://github.com/tidyverse/tidytemplate/blob/main/inst/pkgdown/BS5/extra.scss#L15-L32)

@salim-b
Copy link
Collaborator Author

salim-b commented May 30, 2024

(This is similar, if less dramatic, than what I did in tidytemplate: https://github.com/tidyverse/tidytemplate/blob/main/inst/pkgdown/BS5/extra.scss#L15-L32)

Thanks for the inspiration, appreciated!

dl {margin-bottom: 0rem;}

There is no <dl> element on the reference page... what's the goal of this rule?

h2, .h2 { font-size: 1.75rem; margin-top: 1.5rem;}
h3, .h3 { font-size: 1.2rem; margin-top: 0.5rem;}
h4, .h4 { font-size: 1.1rem}
h5, .h5 { font-size: 1rem}

This results in

Screenshot 2024-05-30 at 16-48-55 Use NocoDB's RESTful APIs

I'm not really convinced by the font size changes.

But adding margin-top to headings is a good thing (regardless of this issue). But I'd rather define them relative to the heading font sizes instead of the root font size, i.e.

h2, h3, h4, h5, h6 {
  margin-top: 1em;
}

// only on non-mobile
@media (min-width: 576px) {
  main {
    h1 {
      margin-top: 1em;
    }
  }
}

@hadley
Copy link
Member

hadley commented May 30, 2024

Not the reference pages, the reference index 😄

I don't have strong feelings about whether it's better to use ems or rems to define the heading margins, but I don't think there's a straightforward relationship between the margin and the font-size.

@hadley
Copy link
Member

hadley commented May 30, 2024

Made #2608 so it's a bit easier to talk about

@salim-b
Copy link
Collaborator Author

salim-b commented May 30, 2024

Presentation/styling is such a subjective matter of taste, I'm not sure we'll ever find common ground... 🥴

but I don't think there's a straightforward relationship between the margin and the font-size.

I think there is. Bigger font-sizes = bigger line height, thus more margin is necessary to achieve the same visual distinction effect.

@hadley
Copy link
Member

hadley commented May 30, 2024

Agreed that bigger sizes = bigger margin, but I don't think it's a linear relationship.

@hadley
Copy link
Member

hadley commented May 30, 2024

Hopefully you agree that #2608 is at least a small improvement 😄

But yes, I don't think we'll ever agree 100% on this issue because I just can't bring myself to add underlines to headings. So I'm going to close it, and you'll just have to be happy with customising your own sites 😄

@hadley hadley closed this May 30, 2024
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.

2 participants