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

[CARE-5792] Fix - Rework Table element to not rely on context #89

Merged

Conversation

kudlajz
Copy link
Contributor

@kudlajz kudlajz commented Jul 22, 2024

No description provided.

kudlajz added 2 commits July 22, 2024 16:09
@kudlajz kudlajz self-assigned this Jul 22, 2024
@kudlajz kudlajz requested review from e1himself and yuriyyakym July 22, 2024 14:46
kudlajz added 2 commits July 23, 2024 13:32

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@kudlajz
Copy link
Contributor Author

kudlajz commented Jul 23, 2024

Hey @yuriyyakym, I've reworked the approach to use the transformation as suggested. Looks much better now I think.

Copy link
Contributor

@yuriyyakym yuriyyakym left a comment

Choose a reason for hiding this comment

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

👍

@kudlajz kudlajz merged commit 6f28e5b into master Jul 23, 2024
8 of 9 checks passed
@kudlajz kudlajz deleted the feature/care-5792-refactor-table-component-in-content-renderer branch July 23, 2024 12:23
Comment on lines +25 to +26
export namespace TableCell {
export type Node = TableCellNode & { isHeader: boolean };
Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, I had a similar challenge in the React emails project, and managed to solve it without extending the model:

https://github.com/prezly/rock-emails/blob/0a96c2d9d10079dc6430fdd07aced3b69c4225fc/lib/renderer/nodes/Table.tsx#L83

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to do it the same way you did, by having a single Table component and rendering everything inside, but the problem was the way Renderer component is implemented, where it iterates over every node in the document and then renders the component that matches.

I think in order to implement it like you did, we would have to change the whole renderer (or maybe change the Renderer to only iterate over top-level nodes) and then have the top-level components render their children separately.

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.

None yet

3 participants