-
Notifications
You must be signed in to change notification settings - Fork 32
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: add renderer component #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an idea about your tests, it's not wrong doing tests for each part – but do you think because at the end they all end up being imported by BlockRenderer, would it be more beneficial to write a comprehensive integration suite for BlockRenderer only? it might save time and be clearer because you could focus on the API it presents rendering different schema sets etc.
Overall looks good though ⭐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get time to test it yet, would love to. Great work 👏
Thanks for the reviews, looks like I have work to do on Monday 😅 |
Move exports to end of files Fix void nodes Move modifier type to Text file Remove wrapper div Rename type Rename block prop types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, will test after merging in main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a workflow now to run the unit tests & linting imo, if you're not too familiar with workflows feel free to make a separate PR and we'll figure it out together ✨
What does it do?
Sets up the basic renderer logic and tests.
Handling the keys (not using indexes) will come in another PR as I want to focus on the essentials before adding complexity.
How to test it
You can try using the BlocksRenderer component in a react application, and give it the reponse of a Strapi app for a blocks field.
You can also verify that it matches our internal RFC for the renderer