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

Improve ReactRenderer types #2011

Merged
merged 1 commit into from
Oct 11, 2021
Merged

Improve ReactRenderer types #2011

merged 1 commit into from
Oct 11, 2021

Conversation

rfgamaral
Copy link
Contributor

This is an attempt to convert the "Mentions" community example to TypeScript. I'm still new to tiptap's codebase, so forgive me if I'm not doing things properly.

Unfortunately I wasn't able to fix every error, so this PR is a bit incomplete. I'm hoping someone can help me figure out the remaining bits:

image
image

@hanspagel
Copy link
Contributor

Thanks for the effort! We decided to not use TypeScript in the examples though. My impression is that only 10 % or less use TypeScript, and they are confused by something they don’t know, but TS users are able to add types on their own.

@philippkuehn What do you think?

@rfgamaral
Copy link
Contributor Author

rfgamaral commented Oct 10, 2021

Thanks for the effort! We decided to not use TypeScript in the examples though. My impression is that only 10 % or less use TypeScript, and they are confused by something they don’t know,

That's understandable, I can revert the example to JavaScript, no problem.

However, this PR also fixes a few types in ReactRenderer, necessary to properly type our client apps. Could you review that, please? I'm not 100% sure about these changes.

but TS users are able to add types on their own.

Except for the errors I posted in the OP, which I have no idea how to fix 😅 Do you think you can help me with that? Maybe further changes to the source types are required? 🤔

@philippkuehn
Copy link
Contributor

@philippkuehn What do you think?

Same!

However, this PR also fixes a few types in ReactRenderer, necessary to properly type our client apps. Could you review that, please? I'm not 100% sure about these changes.

Looks good to me. Could you please update the PR to contain only these changes or create a new one?

@rfgamaral
Copy link
Contributor Author

However, this PR also fixes a few types in ReactRenderer, necessary to properly type our client apps. Could you review that, please? I'm not 100% sure about these changes.

Looks good to me. Could you please update the PR to contain only these changes or create a new one?

Yes, I can update the PR, but those errors need to be fixed first. They show that the changes I'm making here are not finished/correct.

@philippkuehn
Copy link
Contributor

@rfgamaral I fixed another type issue in ReactRenderer right now. I think you stumbled across that issue too.

@rfgamaral
Copy link
Contributor Author

@philippkuehn Pulled your changes, pushed them to this PR, and you can see that there's still one error that needs fixing:

image

@philippkuehn
Copy link
Contributor

@philippkuehn Pulled your changes, pushed them to this PR, and you can see that there's still one error that needs fixing:

image

This is because of conflicting types for our demos since we mix vue and react demos. since we don't want to use TS for our demos we can ignore this for now.

@rfgamaral rfgamaral changed the title Convert 'Mentions' example to TypeScript Improve ReactRenderer types Oct 11, 2021
@rfgamaral
Copy link
Contributor Author

@philippkuehn I've updated the PR, and reverted the TS examples back to JS. Only ReactRenderer changes remain.

@philippkuehn philippkuehn merged commit 31d8ab3 into ueberdosis:main Oct 11, 2021
@philippkuehn
Copy link
Contributor

Thank you!

@rfgamaral rfgamaral deleted the convert-mention-example-to-typescript branch October 11, 2021 18:44
andrewlu0 pushed a commit to trybaseplate/tiptap that referenced this pull request Oct 20, 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.

3 participants