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

Feature/wrapper class #4068

Merged
merged 4 commits into from
May 25, 2023
Merged

Feature/wrapper class #4068

merged 4 commits into from
May 25, 2023

Conversation

patrickbaber
Copy link
Contributor

@patrickbaber patrickbaber commented May 23, 2023

Please describe your changes

We can be proud of what we have built. So let's show this to all devs in the world! I prepend the class name tiptap to the editor element 🎉

How did you accomplish your changes

I added a new method in the Editor class to prepend tiptap in the list of classes. I have updated the docs and demos.

How have you tested your changes

I ran the demos and looked for the class name in the wrapper element. The tests are also working as expected.

How can we verify your changes

You can check the demos and search for the new class name.

Checklist

  • The changes are not breaking the editor
  • Added tests where possible
  • Followed the guidelines
  • Fixed linting issues

@netlify
Copy link

netlify bot commented May 23, 2023

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit b226423
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/646cd2999d4d2100088592b5
😎 Deploy Preview https://deploy-preview-4068--tiptap-embed.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@patrickbaber patrickbaber requested a review from bdbch May 23, 2023 15:48
Copy link
Collaborator

@janthurau janthurau left a comment

Choose a reason for hiding this comment

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

😎 finally we're starting a bit of bragging!!!

@rfgamaral
Copy link
Contributor

Should this be considered a breaking change? Not sure if it's documented anywhere but I'm thinking so, because .ProseMirror class could've been used to stylize certain elements that weren't possible any other way. Not to mention that other ProseMirror internals and/or plugins might introduce .ProseMirror-* classes, for instance .ProseMirror-selectednode and .ProseMirror-trailingBreak. We use this across our products to make sure our editors behave exactly the way we intend them too, and a change like this would be breaking for us. Food for thought.

@janthurau
Copy link
Collaborator

@rfgamaral the .ProseMirror classes are still there, instead of ProseMirror Prosemirror-x you just have tiptap ProseMirror ProseMirror-x

@svenadlung svenadlung self-requested a review May 23, 2023 17:12
@rfgamaral
Copy link
Contributor

@janthurau Thanks for clarifying.

@patrickbaber
Copy link
Contributor Author

For things like styling Tiptap it's a bit inconsistent to use .ProseMirror instead of .tiptap, but as @janthurau mentioned, the old class names of ProseMirror are still existing.

The more specific classes like .ProseMirror-selectednode are completly untouched.

@rfgamaral
Copy link
Contributor

For things like styling Tiptap it's a bit inconsistent to use .ProseMirror instead of .tiptap

IMO, styling should be flexible enough where you can use whatever tooling your project requires without explicitly targetting global classes such as .ProseMirror or .tiptap. However, that's how we initially implemented things (no good reason) and we are now in the process of reviwing that and using our own custom mechanism, but we still have some legacy .ProseMirror usages here and there.

@patrickbaber
Copy link
Contributor Author

@rfgamaral You are right! I mean simpler setups like the ones mentioned here: https://github.com/ueberdosis/tiptap/blob/v2.0.3/docs/guide/styling.md#option-1-style-the-plain-html

@svenadlung svenadlung merged commit 614fc80 into ueberdosis:develop May 25, 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.

4 participants