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: add comment view (for workspace comments, and block comments for partners) #7914

Merged
merged 25 commits into from
Mar 11, 2024

Conversation

BeksOmega
Copy link
Collaborator

The basics

The details

Resolves

Fixes #7905

Proposed Changes

Adds a comment view class which will later be used to

Reason for Changes

Test Coverage

Manually tested:

  • Typing into the block.
  • Setting the text programmatically.
  • Collapsing the block.
  • Collapsing the block programmatically.
  • Setting the block to be non-editable programatically (no way to do this manually).
  • Resizing the block.
  • Resizing the block programatically.
  • Moving the block programatically (no way to do this manually because that will be handled by whatever class is using the view).

Documentation

There will need to be lots of docs later.

Additional Information

Hopefully this is broken up in a way that's easy to review commit-wise 🤞 I do split some methods out into sub-procedures in later commits, but I think that's the only thing that majorly changes between earlier commits and later ones.

@BeksOmega BeksOmega requested a review from a team as a code owner March 7, 2024 21:08
@github-actions github-actions bot added the PR: feature Adds a feature label Mar 7, 2024
core/comments/comment_view.ts Show resolved Hide resolved
core/comments/comment_view.ts Show resolved Hide resolved
core/comments/comment_view.ts Show resolved Hide resolved

// TODO: Remove this comment before merging.
// I think we want comments to exist on the same layer as blocks.
workspace.getLayerManager()?.append(this, layers.BLOCK);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The disadvantage of being on the same layer as blocks is that a block can go over the comment and look like it's inside it. That's a bigger issue with mutators bubbles, since they always hold blocks. I'm neutral on which one we should do for comments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is also a weird thing where when we deserialize, the comments will be deserialized after blocks, so they'll always end up on top after load even if they were behind blocks before.

core/comments/comment_view.ts Outdated Show resolved Hide resolved
core/comments/comment_view.ts Outdated Show resolved Hide resolved
svgRoot,
);
// TODO: Before merging, does this mean to override an individual image,
// folks need to replace the whole media folder?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you have thoughts on this @rachel-fenichel ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know. Is replacing those images an immediate need, or a theoretical one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a need for spork blocks. For MakeCode we can just include their image in core.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@maribethb as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, they do, but tbh they probably should anyway because the default one links to one hosted on app engine. I don't see this as being a concern, it's how the rest of media assets work. If we wanted to refactor that in the future that's a different discussion and shouldn't be folded into this work imo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok I'll dig into what MakeCode is currently doing and chat with them if it seems like it might be a problem. Since we've been holding v11 until we can have stuff ready for MakeCode, I want to make sure it actually is ready, and they're not going to have to wait for a v12.

Copy link
Contributor

Choose a reason for hiding this comment

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

What kind of problems are you thinking about? The media directory is already configurable via options, and they can copy the assets they want to keep the same into their own hosting solution (which is what we currently recommend, though not that loudly). I feel like I'm missing something here if you're worried about something needing a v12.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right I don't think this would need a v12. If anything I would just make it possible to configure the assets individually (which would be a feat not breaking). Based on some investigation it looks like they're already hosting everything themselves so it shouldn't be an issue. I just want to make sure we have everything pinned down so we can actually say MakeCode has been migrated.

@BeksOmega BeksOmega merged commit fc4228c into google:rc/v11.0.0 Mar 11, 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.

3 participants