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: add renderer component #3

Merged
merged 14 commits into from
Nov 2, 2023
Merged

feat: add renderer component #3

merged 14 commits into from
Nov 2, 2023

Conversation

remidej
Copy link
Contributor

@remidej remidej commented Oct 26, 2023

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

@remidej remidej marked this pull request as ready for review October 26, 2023 16:04
Copy link
Contributor

@joshuaellis joshuaellis left a 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 ⭐

src/BlocksRenderer/BlocksRenderer.tsx Outdated Show resolved Hide resolved
src/BlocksRenderer/BlocksRenderer.tsx Outdated Show resolved Hide resolved
src/Text/Text.tsx Outdated Show resolved Hide resolved
src/Text/Text.tsx Outdated Show resolved Hide resolved
src/Text/Text.tsx Outdated Show resolved Hide resolved
src/Block/Block.tsx Outdated Show resolved Hide resolved
src/Block/Block.tsx Outdated Show resolved Hide resolved
Copy link

@madhurisandbhor madhurisandbhor left a 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 👏

src/Text/Text.tsx Outdated Show resolved Hide resolved
src/Text/Text.tsx Outdated Show resolved Hide resolved
src/BlocksRenderer/BlocksRenderer.tsx Outdated Show resolved Hide resolved
src/BlocksRenderer/BlocksRenderer.tsx Outdated Show resolved Hide resolved
src/BlocksRenderer/BlocksRenderer.tsx Outdated Show resolved Hide resolved
src/BlocksRenderer/BlocksRenderer.tsx Outdated Show resolved Hide resolved
src/BlocksRenderer/BlocksRenderer.tsx Outdated Show resolved Hide resolved
src/Block/Block.tsx Outdated Show resolved Hide resolved
src/Block/Block.tsx Outdated Show resolved Hide resolved
src/Block/Block.tsx Outdated Show resolved Hide resolved
@remidej
Copy link
Contributor Author

remidej commented Oct 27, 2023

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
src/Block.tsx Outdated Show resolved Hide resolved
src/Block.tsx Outdated Show resolved Hide resolved
@remidej remidej requested a review from joshuaellis October 31, 2023 09:48
@remidej remidej changed the title Setup renderer components and tests feat: add renderer component Oct 31, 2023
Copy link

@madhurisandbhor madhurisandbhor left a 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

Copy link
Contributor

@joshuaellis joshuaellis left a 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 ✨

@remidej remidej merged commit 8627a24 into main Nov 2, 2023
@remidej remidej deleted the feature/renderer branch November 2, 2023 08:35
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