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

Remove typography styles from not-prose elements in addition to their children #301

Merged
merged 3 commits into from
Aug 30, 2023

Conversation

jonnitto
Copy link
Contributor

@jonnitto jonnitto commented Mar 2, 2023

Close #299

@vercel
Copy link

vercel bot commented Mar 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
tailwindcss-typography ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 30, 2023 5:42pm

@adamwathan
Copy link
Member

For others on the team when thinking through this — we’ll want to think through the impact of this as a breaking change, since adding this class to an element like blockquote would remove margins now.

@jonnitto
Copy link
Contributor Author

jonnitto commented Mar 2, 2023

Perhaps we could make this configurable?

@jonnitto
Copy link
Contributor Author

jonnitto commented Mar 2, 2023

I have a working prototype for that.

The plugin options would look something like that…

module.exports = plugin.withOptions(
  ({ className = 'prose', target = 'modern', includeNotProsedElement = false } = {}) => {
    return function ({ addVariant, addComponents, theme, prefix }) {
      let modifiers = theme('typography')

     
      let options = { className, prefix, includeNotProsedElement }

But how you would name such a setting? Naming is hard 😉

@reinink reinink changed the title Ignore not-prose also on the element itself Remove typography styles from not-prose elements in addition to their children Aug 30, 2023
@reinink
Copy link
Member

reinink commented Aug 30, 2023

Hey thanks for this contribution @jonnitto!

@jonnitto jonnitto deleted the issue/299 branch September 8, 2023 14:12
@robdekort
Copy link

Hey folks. This turned out a breaking change for my sites and a widely used Starter Kit I run for Statamic as I used the not-prose class to add vertical margins to elements within the prose class.

Maybe I could use that config option, but tbh I'm not really sure how to implement that. Is there someone willing to explain this a bit?

Thanks a bunch in advance! 🙌

@robdekort
Copy link

Never mind, figured out a different way of doing this going forward.

@Strajk
Copy link

Strajk commented Oct 2, 2023

Heya, this change turned out to be pretty breaking for Protocol template

Repro: Download protocol, install deps, should be broken with 5.10

Screen 2023-10-02 at 08 19 32

@b5
Copy link

b5 commented Oct 9, 2023

Is there recommended fix for not-prose in the protocol template moving forward? I'm totally fine with the breaking change, but it'd be great to have a recommended approach, even just a set of tailwind classes to drop into an inner-div from a not-prose parent would work

@thecrypticace
Copy link
Contributor

thecrypticace commented Oct 10, 2023

@Strajk @b5 hey yeah there is. We spent a little bit of time figuring out a simple update to the template to work around these changes in the typography plugin. We've updated our live preview as well as the download link for the template to address these issues caused by the change.

A summary of what we've done is:

  1. In typography.ts, we removed the > * block of styles from the config
  2. In src/components/Prose.ts, we added classes that apply these same styles to the <Prose> component using the variant [html_:where(&>*)]. This gives them the right specificity to work the same way they did before while generally fixing the problem with not-prose.
  3. In src/components/Code.tsx, we removed not-prose from containerClassName
  4. In src/components/Code.tsx, we wrapped the headers and panels in a <div className="not-prose">

The diffs look like this:

  1. In typography.ts:
- // Layout
- '> *': {
-   maxWidth: theme('maxWidth.2xl'),
-   marginLeft: 'auto',
-   marginRight: 'auto',
-   '@screen lg': {
-     maxWidth: theme('maxWidth.3xl'),
-     marginLeft: `calc(50% - min(50%, ${theme('maxWidth.lg')}))`,
-     marginRight: `calc(50% - min(50%, ${theme('maxWidth.lg')}))`,
-   },
- },
  1. In src/components/Prose.ts:
    <Component
-      className={clsx(className, 'prose dark:prose-invert')}
+      className={clsx(
+        className,
+        'prose dark:prose-invert',
+        // `html :where(& > *)` is used to select all direct children without an increase in specificity like you'd get from just `& > *`
+        '[html_:where(&>*)]:mx-auto [html_:where(&>*)]:max-w-2xl [html_:where(&>*)]:lg:mx-[calc(50%-min(50%,theme(maxWidth.lg)))] [html_:where(&>*)]:lg:max-w-3xl',
+      )}
      {...props}
    />
  1. In src/components/Code.tsx
   let containerClassName =
-    'not-prose my-6 overflow-hidden rounded-2xl bg-zinc-900 shadow-md dark:ring-1 dark:ring-white/10'
+    'my-6 overflow-hidden rounded-2xl bg-zinc-900 shadow-md dark:ring-1 dark:ring-white/10'
  1. In src/components/Code.tsx
  <Tab.Group {...tabGroupProps} className={containerClassName}>
-    {header}
-    {panels}
+    <div className="not-prose">
+      {header}
+      {panels}
+    </div>
  </Tab.Group>
  <div className={containerClassName}>
-    {header}
-    {panels}
+    <div className="not-prose">
+      {header}
+      {panels}
+    </div>
  </div>

Hope that helps and let us know if you notice anything else! ✨

@rubendinho
Copy link

rubendinho commented Oct 17, 2023

This was a breaking change for our docs site as well. I can appreciate that this option makes sense for many people, but it really doesn't make sense to add potentially breaking behavior changes into a patch update, given that the project follows Semantic Versioning.

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.

not-prose doesn't ignore the element itself
8 participants