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

feat(react): remove empty elements from rendering #6

Merged
merged 4 commits into from
May 28, 2021
Merged

feat(react): remove empty elements from rendering #6

merged 4 commits into from
May 28, 2021

Conversation

rpfaeffle
Copy link
Contributor

@rpfaeffle rpfaeffle commented May 26, 2021

This PR adds support for removing specified elements if they would be rendered empty

Maybe the naming of the property determining whether an empty element should be rendered or not could be changed, it's not as descriptive as it could be. (I did not find another name though)

@changeset-bot
Copy link

changeset-bot bot commented May 26, 2021

🦋 Changeset detected

Latest commit: c6be572

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@graphcms/rich-text-types Patch
@graphcms/rich-text-react-renderer Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jpedroschmitz
Copy link
Member

Hey @KaterBasilisk6, thanks a lot for taking a look into this! I really appreciate your help with the project. ❤️

I've left a comment in #5 about this, but I think it would be better to remove empty headings since this will affect SEO, and we don't want this. About the other elements, I don't see any problem in rendering if empty.

Adding a prop for this may confuse new users, especially if we don't clarify what this is about and why it's important. That's why I prefer not to add a prop for this right now.

What do you think? We can keep your code and remove the prop you've added.

@jpedroschmitz jpedroschmitz linked an issue May 27, 2021 that may be closed by this pull request
@jpedroschmitz jpedroschmitz added the enhancement New feature or request label May 27, 2021
@rpfaeffle
Copy link
Contributor Author

Hey @jpedroschmitz,

I see your point that a prop could confuse new users. I'm also seeing your point that checking headings would be enough.

The only enhancement to my code I would suggest would be writting a function instead of using filter or just looking into the first element to check for empty text elements, because this would decrease rendering time (worst case O(1), instead of O(n)) for styled headings (bold, italic, underlined).

Will you remove the prop or should I do it?

@jpedroschmitz
Copy link
Member

You can remove it if you can.

I would also ask you to increase the size limit to 5kb here. This should fix the size workflow error.

@rpfaeffle
Copy link
Contributor Author

Should I keep the list or should I remove it completly?

@jpedroschmitz
Copy link
Member

Keep it. If we decide to add more elements to be checked, we can add them to the list.

It would be nice to add a note about this in the Readme also. Something like this:

By default, we remove empty headings from the elements list to prevent SEO issues.

Copy link
Member

@jpedroschmitz jpedroschmitz left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your help in this @KaterBasilisk6!

I'll be adding changesets and releasing it soon =D

@rpfaeffle
Copy link
Contributor Author

You welcome @jpedroschmitz. I'm glad I was able to help.

@jpedroschmitz
Copy link
Member

Hey @KaterBasilisk6, could you allow me to add commits?
You can do so by enabling edits from maintainers. Check this tutorial.

@rpfaeffle
Copy link
Contributor Author

Hey @jpedroschmitz,

I thought I would have enabled it already. I reanabled it again.

@jpedroschmitz
Copy link
Member

It worked now. No idea why, haha

Thanks again!

@jpedroschmitz jpedroschmitz merged commit 23b87f6 into hygraph:main May 28, 2021
@github-actions github-actions bot mentioned this pull request May 28, 2021
@rpfaeffle rpfaeffle deleted the feat/remove-empty-elements branch June 11, 2021 06:59
@jpedroschmitz jpedroschmitz added the react-renderer Issues and PR's related to @graphcms/rich-text-react-renderer label Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request react-renderer Issues and PR's related to @graphcms/rich-text-react-renderer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RichText renders empty elements resulting in broken design
2 participants