Skip to content

feat: mini workspace bubble #7096

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

Merged
merged 9 commits into from
May 19, 2023

Conversation

BeksOmega
Copy link
Collaborator

@BeksOmega BeksOmega commented May 18, 2023

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Fixes #7045

Proposed Changes

Adds a new kind of bubble which is a mini workspace bubble.

Reason for Changes

We want bubbles to be pure views with a object oriented structure. This works toward that =)

Test Coverage

Manually tested by hacking into the warning icon. Looks the same as bubbles did previously.

This is the one I'm most nervous about since it has so much functionality that I can't test it all. But the simple case looks good! And I added validation to hopefully restrict peeps to those use cases.

If we want we can also mark the constructor @internal until we're more confident in it.

Documentation

N/A

Additional Information

@BeksOmega BeksOmega changed the title Feat/mini workspace bubble feat: mini workspace bubble May 18, 2023
@github-actions github-actions bot added the PR: feature Adds a feature label May 18, 2023
@BeksOmega BeksOmega marked this pull request as ready for review May 18, 2023 17:16
@BeksOmega BeksOmega requested a review from a team as a code owner May 18, 2023 17:16
@BeksOmega BeksOmega requested a review from cpcallen May 18, 2023 17:16
@github-actions github-actions bot removed the PR: feature Adds a feature label May 18, 2023
@github-actions github-actions bot added the PR: feature Adds a feature label May 18, 2023
Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

We want bubbles to be pure views with a object oriented structure. This works toward that =)

I'm not quite sure what you mean by "pure views".

Am I correct in understanding that the current Mutator (really: block button with gear icon) will in future use a MiniWorkspaceBubble instead of whatever it currently uses?

This is the one I'm most nervous about since it has so much functionality that I can't test it all. But the simple case looks good! And I added validation to hopefully restrict peeps to those use cases.

I was surprised that the mini workspace doesn't support scrolling, trash, etc. Are there technical limitations why that is, or is this just policy? If the latter, why?

If we want we can also mark the constructor @internal until we're more confident in it.

That seems like a good idea, though not exporting it (as is presently the case) is probably just as effective.

Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

We want bubbles to be pure views with a object oriented structure. This works toward that =)

I'm not quite sure what you mean by "pure views".

Am I correct in understanding that the current Mutator (really: block button with gear icon) will in future use a MiniWorkspaceBubble instead of whatever it currently uses?

This is the one I'm most nervous about since it has so much functionality that I can't test it all. But the simple case looks good! And I added validation to hopefully restrict peeps to those use cases.

I was surprised that the mini workspace doesn't support scrolling, trash, etc. Are there technical limitations why that is, or is this just policy? If the latter, why?

If we want we can also mark the constructor @internal until we're more confident in it.

That seems like a good idea, though not exporting it (as is presently the case) is probably just as effective.

@BeksOmega
Copy link
Collaborator Author

Am I correct in understanding that the current Mutator (really: block button with gear icon) will in future use a MiniWorkspaceBubble instead of whatever it currently uses?

Yes!

I was surprised that the mini workspace doesn't support scrolling, trash, etc. Are there technical limitations why that is, or is this just policy? If the latter, why?

There are technical limitations related to how the workspace gets sized (and automatically resized). We can't support scrolling because the workspace should never be scrollable (always resizing to show the whole workspace). We can't support the trashcan because (currently) we don't take the size of the trashcan into account when resizing.

In the future we can always add support for these things! I just don't have time to implement that right now.

That seems like a good idea, though not exporting it (as is presently the case) is probably just as effective.

I think we do want to export this for documentation purposes though. I'm pretty sure if it's not exported then API extractor won't include information about what other methods it has and what not. I'll mark it as internal just in case.

@cpcallen
Copy link
Contributor

I think we do want to export this for documentation purposes though. I'm pretty sure if it's not exported then API extractor won't include information about what other methods it has and what not. I'll mark it as internal just in case.

Does API extractor include information about things marked @internal in its output? And does it have any way to distinguish the type from the constructor for the type? I'd sort of expect it would either show nothing about the type, or it would show the whole type as being @internal, which I think is not what you want.

But if you've checked that it is output as you intend then great.

Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

Approved subject to due consideration given to remaining unresolved comments (changes at your discretion).

@BeksOmega
Copy link
Collaborator Author

Does API extractor include information about things marked @internal in its output? ...

I will resolve this as part of #7082

@BeksOmega BeksOmega merged commit 83c6c73 into google:develop May 19, 2023
@BeksOmega BeksOmega deleted the feat/mini-workspace-bubble branch May 14, 2024 16:27
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.

Refactor the "mutator bubble" to be a MiniWorkspaceBubble
2 participants