Directory

RichTextValue: Typescript Adjustment by margolisj · Pull Request #54002 · WordPress/gutenberg · GitHub
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.

Sign up for GitHub

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

Jump to bottom

RichTextValue: Typescript Adjustment #54002

Merged
merged 3 commits into from
Aug 29, 2023
Merged

RichTextValue: Typescript Adjustment #54002

merged 3 commits into from
Aug 29, 2023

Conversation

margolisj
Copy link
Contributor

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

@margolisj
Copy link
Contributor Author

margolisj commented Aug 28, 2023 •
edited

@noahtallen Saw you working on this in #49651. Was there a reason why start and/or end should be marked as undefined. I couldn't find anything.

I threw in another small JSDoc type error I had noticed while using this package as well.

@noahtallen
Copy link
Member

I think I just copy/pasted the undefined portion from where it was originally added. So we should be ok, as long as no where in our code expects it to handle undefined.

@margolisj
Copy link
Contributor Author

Really appreciate the quick response @noahtallen 👍

noahtallen
noahtallen approved these changes Aug 29, 2023
View reviewed changes
Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

I triggered the e2e tests again -- should be unrelated. Otherwise this is looking good! I'll merge once the checks pass.

@noahtallen noahtallen enabled auto-merge (squash) August 29, 2023 20:58
@noahtallen noahtallen added [Type] Enhancement A suggestion for improvement. [Package] Rich text /packages/rich-text labels Aug 29, 2023
@noahtallen noahtallen merged commit 5dbe0da into WordPress:trunk Aug 29, 2023
50 checks passed
@github-actions github-actions bot added this to the Gutenberg 16.6 milestone Aug 29, 2023
@margolisj
Copy link
Contributor Author

I triggered the e2e tests again -- should be unrelated. Otherwise this is looking good! I'll merge once the checks pass.

Thanks!

ellatrix
ellatrix reviewed Aug 30, 2023
View reviewed changes
packages/rich-text/src/types.ts
start: number | undefined;
end: number | undefined;
start: number;
end: number;
Copy link
Member

Choose a reason for hiding this comment

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

Why has this been removed? These are optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are they optional properties i.e. a typescript start?: number Or optionally set as undefined? Would you mind linking code where this would be set as undefined? My bad I must have totally missed it. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Probably both. A rich text instance may not have selection

gutenberg/packages/block-editor/src/components/rich-text/index.js

Lines 149 to 150 in f706c86

selectionStart: isSelected ? selectionStart.offset : undefined,
selectionEnd: isSelected ? selectionEnd.offset : undefined,

gutenberg/packages/rich-text/src/component/use-input-and-selection.js

Line 269 in f706c86

const index = undefined;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

O boy, looks like my eyes deceived me. Mind explaining to me the context for which you'd end up with an undefined value? In my usage I have never been able to get an undefined value appear when there is no selection or when there is. Attaching a short video:

Screen_Recording_2023-08-30_at_3.52.39_PM.mov

Would you like me to push another revert commit to put back the optional undefined? Are you good with the other change that was in this commit (the null value for className)?

While I have you attention, through for the wrong reason sadly, has there been any discussion about porting this package to typescript? If so, was there any blockers etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ellatrix bumping this. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

All rich text instances have an undefined selection unless it is selected. So if you select one paragraph, all the other paragraph's rich text instances have an undefined selection except for the selected paragraph's.

@margolisj
Copy link
Contributor Author

@noahtallen I pulled in the updated types today. I believe name in settings should also be optional since it is overriding the value at the start of the function if it is defined via the first parameter.

@margolisj margolisj deleted the rich-text-no-undefined branch September 12, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ellatrix ellatrix ellatrix left review comments

@noahtallen noahtallen noahtallen approved these changes

@fluiddot fluiddot Awaiting requested review from fluiddot fluiddot is a code owner

@dcalhoun dcalhoun Awaiting requested review from dcalhoun dcalhoun is a code owner

Assignees
No one assigned
Labels
[Package] Rich text /packages/rich-text [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
Gutenberg 16.6
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@margolisj @noahtallen @ellatrix