Skip to content
This repository has been archived by the owner on Feb 11, 2025. It is now read-only.

Add and apply TailwindCSS Typography plugin #202

Closed
wants to merge 4 commits into from

Conversation

podliashanyk
Copy link
Contributor

@podliashanyk podliashanyk commented Oct 24, 2024

Relevant for Uninett/Argus#1054

Discussion:

  • should typography plugin be activated (current implementation) or deactivated on the <main> by default?

@podliashanyk podliashanyk requested review from a team October 24, 2024 08:32
@podliashanyk podliashanyk self-assigned this Oct 24, 2024
@elfjes
Copy link
Contributor

elfjes commented Oct 24, 2024

What's this about?

@podliashanyk
Copy link
Contributor Author

What's this about?

Tailwind typography provides pretty and reasonable default styles for a set of HTML tags https://github.com/tailwindlabs/tailwindcss-typography. When combined with daisyUI we will get default results like here: https://daisyui.com/docs/layout-and-typography/#-1.

In #190 (comment) we are discussing adding page headings to pages other than /incidents.

This PR also applies typography styles to <main> by default, yet it is possible to deactivate it like so: https://github.com/Uninett/argus-htmx-frontend/blob/5106dc682a65a8e74fb5ed6be597669d925ff74c/src/argus_htmx/templates/htmx/incidents/_base.html#L4C1-L6C4

Copy link
Contributor

@elfjes elfjes left a comment

Choose a reason for hiding this comment

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

My initial/gut reaction about using @tailwindcss/typography: Let's be really careful before adding this.

Reading the docs, it seems that the prose class is meant for larger sections that have multiple headings, paragraphs, etc (such as content coming from a CMS or rendered markdown). It's called "prose" for a reason. The docs explicitly say that you probably don't want default prose styling, and I agree. The result is that now you need to not-prose large sections of the page, which seems like an antipattern.

If we want to apply some styles to headings because we want a heading system, that is fine, but I'd suggest creating classes for that and not just add style to tags by default.

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

I read your comments about the typography plugin in my PR, but I'm not sure I understand the reasoning, given how the plugin is explained.

We're not about to add large sections of prose to Argus, so I find this a bit overkill just to style some page headings.

@podliashanyk
Copy link
Contributor Author

podliashanyk commented Oct 24, 2024

Agree with the comments above, furthermore putting it on <main> restricts max width to 65ch so a bad idea altogether.

Regarding the looks itself, I personally like the following compiled typography styles (for our headings system and for Uninett/Argus#1054):

  • font size, line-height etc
  • all headings tags
  • strong

With such a little subset of relevant styles let's just add our own classes.

Although some typography plugin styles (for example for a) are better for accessibility in general but it's minor improvements and not worth adding the whole thing just because of it IMO.. :)

@hmpf
Copy link
Collaborator

hmpf commented Oct 29, 2024

Should our styles have a prefix? "argus-"?

@hmpf
Copy link
Collaborator

hmpf commented Nov 28, 2024

I'm closing this...

@hmpf hmpf closed this Nov 28, 2024
@hmpf hmpf deleted the add-typography-plugin branch February 11, 2025 08:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants