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

pager improvements #192

Merged
merged 4 commits into from
Nov 17, 2023
Merged

pager improvements #192

merged 4 commits into from
Nov 17, 2023

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Nov 17, 2023

This…

  • Adds the top-level /index as the implicit first page (if there’s a title set)
  • Fixes escaping when interpolating user content into HTML (no automatic protection here!)
  • Switches to the blue (focus color) border on hover for pager buttons
  • Produces “pretty” URLs by truncating /index as appropriate
  • Avoids possible collision with userland CSS by using observablehq- prefixed ids
  • Improves the type safety of the pager function
  • Adds unit tests for the pager function

Example:
Screenshot 2023-11-16 at 10 41 03 PM

@mbostock mbostock requested review from Fil and trebor November 17, 2023 06:44
@Fil
Copy link
Contributor

Fil commented Nov 17, 2023

Related (though not a comment on this PR _per se_), as I was just working on the sidebar, I wanted to change the behavior wrt the **title** config option. For me, it's surprising that this option would make the top link to / appear (or disappear).

I'd like to suggest instead that the title option just changes the text of that link, which would be always present (unless of course there is only 1 page).

Otherwise, we must fix the bug that makes the whole navigation menu bold if there is no title. (This bug is already present in main; my suggestion fixes by way of consequence.)

When the first link goes to the home page (which in my suggestion would be always), we should skip pages[0] if it is /index, to avoid a repetition.

This in turn would allow the prev/next links to include the /index iff the user adds that page as the first element of config.pages (it would be the default if you don't specify config.pages).

this comment is now #195

@Fil
Copy link
Contributor

Fil commented Nov 17, 2023

My second comment is on the use of #observable-prev and #observable-next. I don't really like the markup it generates, and I'm not sure how this clashes with userland css, since everything was scoped to #observablehq-footer?

If we wanted to not use the classes .prev and .next, we could maybe use a (more semantic) a[rel=prev] markup?

Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

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

I'll act on my first comment #192 (comment) in a separate PR.

Second comment about the markup is a non-blocking question.

@mbostock
Copy link
Member Author

mbostock commented Nov 17, 2023

My second comment is on the use of #observable-prev and #observable-next. I don't really like the markup it generates, and I'm not sure how this clashes with userland css, since everything was scoped to #observablehq-footer?

What do you not like about the markup? I do like the rel=prev/next semantic approach though!

As for clashing, it clashes if users define their own .prev or .next classes. For example if a user says .prev { color: red; } then the footer previous button is red. We shouldn’t expect the user to know which classes our UI uses. Instead we always use the observablehq- prefix to defend against user styles. (User styles on global elements can still affect us, but that’s unavoidable without shadow DOM.)

I agree with the title and /index in pages comments. Thanks for volunteering to improve that.

@mbostock mbostock force-pushed the mbostock/index-next branch from 148e00b to 5cccbbf Compare November 17, 2023 08:19
@mbostock mbostock merged commit 00cd080 into main Nov 17, 2023
1 check passed
@mbostock mbostock deleted the mbostock/index-next branch November 17, 2023 08:20
@Fil Fil mentioned this pull request Nov 17, 2023
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