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

KDS-1416 Migrate RTE & Co #4255

Merged
merged 21 commits into from
Nov 8, 2023
Merged

KDS-1416 Migrate RTE & Co #4255

merged 21 commits into from
Nov 8, 2023

Conversation

gyfchong
Copy link
Contributor

@gyfchong gyfchong commented Nov 1, 2023

Why

Migrate RichTextEditor, RichTextContent, EditableRichTextContent from kaizen-legacy

Notes

  • @cultureamp/rich-text-toolkit added as an optional Peer Dep, meaning consumers can choose not to install the package if they do not use RTE. This does break the circular dependency as we are no longer trying to bundle RTT, however Ideally we would want to move towards removing this peer dep altogether.

@gyfchong gyfchong requested a review from a team as a code owner November 1, 2023 07:21
Copy link

changeset-bot bot commented Nov 1, 2023

🦋 Changeset detected

Latest commit: b275bcf

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

This PR includes changesets to release 1 package
Name Type
@kaizen/components Minor

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

Copy link
Contributor

github-actions bot commented Nov 1, 2023

✨ Here is your branch preview! ✨

Last updated for commit b275bcf: Merge branch 'main' into rte-rtt-optional-peer-dep-poc

mcwinter07
mcwinter07 previously approved these changes Nov 2, 2023
Copy link
Contributor

@mcwinter07 mcwinter07 left a comment

Choose a reason for hiding this comment

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

I've just left a few comments some stories, more around uplift and things we could improve the docs later but the migration looks hunky dory to me so giving this a ✅

<Canvas of={RichTextEditorStories.DefaultValue} />

#### Malformed Content
If your default value does not conform correctly, the RTE will throw an error.
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also callout the onDataError callback that's available to consumers if they want to catch and handle malformed data


type Story = StoryObj<typeof meta>

export const Playground: Story = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential improvement later is to add a drop down for controls to let our consumer add items to the toolbar in the playground story

parameters: {
docs: {
canvas: {
sourceState: "shown",
Copy link
Contributor

Choose a reason for hiding this comment

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

One call out that Dale mentioned a while back was that when he first used the RTE, it was that it wasn't clear to him that handleOnChange would need to setter with editorState.toJSON().doc.content.

Something we could either document later or expose in the source code of the canvas

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 can do a hardcode here again 👍


## API

<DocsStory of={RichTextEditorStories.Controls} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to circle back later and add more to this in terms of current controls available and how to handle grouping

@gyfchong
Copy link
Contributor Author

gyfchong commented Nov 2, 2023

Might pause this and wait for the RTT to be migrated first, then we won't be shipping a massive RTE and the peer dep wouldn't be necessary

@gyfchong gyfchong merged commit 6500eb9 into main Nov 8, 2023
15 checks passed
@gyfchong gyfchong deleted the rte-rtt-optional-peer-dep-poc branch November 8, 2023 02:06
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