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

Add and apply TailwindCSS Typography plugin #202

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

podliashanyk
Copy link
Contributor

@podliashanyk podliashanyk commented Oct 24, 2024

Relevant for #171

Discussion:

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

@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 #171):

  • 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-"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Changes requested
Development

Successfully merging this pull request may close these issues.

4 participants