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

Bugfix/set node #4143

Closed
wants to merge 8 commits into from
Closed

Conversation

adisen
Copy link

@adisen adisen commented Mar 26, 2021

Description
This PR allows Transforms.setNodes to take either a Partial as it second argument to props (which we have before) which overrides the props on the node or a function that takes the existing props for the relevant node and returns a new set of props to attach to the node.

Issue
Fixes: #3808

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)

@@ -1,3 +1,4 @@
import { update } from 'lodash'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah it looks like we aren't using update yet in this code, maybe it's in code you haven't pushed yet?

Copy link
Author

Choose a reason for hiding this comment

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

I didn't use it too 🤔

@changeset-bot
Copy link

changeset-bot bot commented Mar 30, 2021

⚠️ No Changeset found

Latest commit: 1605e41

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@adisen
Copy link
Author

adisen commented Mar 30, 2021

@timbuckley, can you please help me take a look at this? I'm not exactly sure what the problem is

@nivekithan
Copy link
Contributor

if you are talking about the failing test then the problem is in this file docs/concepts/11-typescript.md. Just to go this file and save it which will remove a line.
That fixed the problem for me #4150

@adisen
Copy link
Author

adisen commented Mar 30, 2021

if you are talking about the failing test then the problem is in this file docs/concepts/11-typescript.md. Just to go this file and save it which will remove a line.
That fixed the problem for me #4150

Thank you. But the file isn't part of this branch.

@princiya
Copy link

princiya commented Apr 1, 2021

@adisen based on @nivekithan 's comment, you might want to refer to this commit where the docs/concepts/11-typescript.md had to be fixed despite not being used in the PR itself.

@adisen
Copy link
Author

adisen commented Apr 1, 2021

@adisen based on @nivekithan 's comment, you might want to refer to this commit where the docs/concepts/11-typescript.md had to be fixed despite not being used in the PR itself.

I understand. But I can't even find the file in the repo, so there's no way of fixing it

@ianstormtaylor
Copy link
Owner

@adisen it might have gotten removed in subsequent changes. If you pull the latest changes to main and merge them into your branch (resolving any conflicts that occur) that should eliminate the error.

@adisen adisen marked this pull request as ready for review April 7, 2021 16:09
@adisen
Copy link
Author

adisen commented Apr 7, 2021

@timbuckley @BrentFarese @princiya please take a look. Thanks

@@ -67,7 +68,7 @@ export interface NodeTransforms {
) => void
setNodes: <T extends Node>(
editor: Editor,
props: Partial<Node>,
props: Partial<T> | ((existingProps: NodeProps) => Partial<T>),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to change the type of NodeProps to be generic. Can you try the below and let me know if it works?

type NodeProps<T extends Node> = T extends Element ? Omit<T, 'children'> : Omit<T, 'text'>;

Copy link
Author

Choose a reason for hiding this comment

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

I'll change this in the interfaces/node.ts file right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wherever the type is defined change it there. Thx.

@@ -1,13 +1,11 @@
---

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a lot of diff in this PR that you probably did not change? Rebase off Main to get rid of the diff if you can. Thanks.

Copy link
Collaborator

@BrentFarese BrentFarese left a comment

Choose a reason for hiding this comment

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

Try and make NodeProps type generic. Also try and rebase off Main to get rid of diff/changes that you probably did not make in this PR. Otherwise, test and other changes look good! Thanks.

---

**Description**
---**Description**
Copy link

@princiya princiya Apr 7, 2021

Choose a reason for hiding this comment

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

we did a rebase together off the main branch. looks like the yarn fix command broke this? @adisen and I will check again...
when the lint errors were fixed previously, it was based against the outdated MLH repository's master branch. reverting these files might most likely fix the unwanted diffs.

@adisen
Copy link
Author

adisen commented Apr 16, 2021

I'm closing this cause of the rebasing issues. And I have opened another PR that solves the problem and keeps everything clean

@adisen adisen closed this Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transform.setNodes improvement and bug
7 participants