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: allow overriding comment icons #7937

Merged
merged 5 commits into from
Mar 15, 2024

Conversation

BeksOmega
Copy link
Collaborator

The basics

The details

Resolves

Fixes #7910
Fixes #7911

Proposed Changes

Allows comment icons to be overriden, and constructed from Block.prototype.setCommentText.

Reason for Changes

This will allow external partners to create their own comment icons (e.g. that use the new CommentView, while still using the built-in functionality like setCommentText, context menu options, etc.

Test Coverage

Added tests that setCommentText properly constructs classes from the registry.

Documentation

Yesh, we'll need to document the fact that you can do this somewhere.

Additional Information

N/A

@BeksOmega BeksOmega requested a review from a team as a code owner March 14, 2024 20:21
@BeksOmega BeksOmega requested a review from maribethb March 14, 2024 20:21
@github-actions github-actions bot added the PR: feature Adds a feature label Mar 14, 2024
return (
isIcon(obj) &&
(obj as any)['setText'] !== undefined &&
(obj as any)['getText'] !== undefined &&
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Forgot to check for setBubbleSize and getBubbleSize

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I thought this was intentional because if we're checking from Block then it could be headless so they don't need to care about bubble size necessarily. But I guess if they only care about headless they prooobably don't have a need to override the class to begin with?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Headless ones also need to have these methods so that the size properly gets round-tripped through serialization.

I also didn't think they were necessary, and I actually left those methods out originally. I only added them to the interface when I ran into the compiler errors :P Which is why I forgot to add them to the type guard.

@BeksOmega BeksOmega enabled auto-merge (squash) March 15, 2024 18:09
@BeksOmega BeksOmega merged commit 8821c83 into google:rc/v11.0.0 Mar 15, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants