chore: remove some underscores in icon-related classes #6583

rachel-fenichel · 2022-10-26T22:28:00Z

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

Part of #6548

Proposed Changes

Remove underscores from private property names and method names, where they weren't being used by tests.
Also cleans up some nullability indicators.

Behavior Before Change

No change.

Behavior After Change

No change.

Reason for Changes

General cleanup, as a way of reading through code and understanding icon architecture.

Test Coverage

Mocha tests, and also opened up the playground and played around with comments and mutators.

Documentation

None.

Additional Information

comment.textarea_ and comment.paragraphElement_ are being accessed directly in tests, so I left them alone.

maribethb · 2022-10-26T23:54:10Z

core/comment.ts

    const doubleBorderWidth = 2 * Bubble.BORDER_WIDTH;
    const widthMinusBorder = size.width - doubleBorderWidth;
    const heightMinusBorder = size.height - doubleBorderWidth;
-    this.foreignObject_!.setAttribute('width', `${widthMinusBorder}`);
-    this.foreignObject_!.setAttribute('height', `${heightMinusBorder}`);
+    this.foreignObject!.setAttribute('width', `${widthMinusBorder}`);


in the interest of type safety I think it'd be reasonable to add a if (!this.foreignObject || !this.textarea_) return; at the top and cut out the non-null assertions

maribethb · 2022-10-26T23:59:50Z

core/icon.ts

@@ -175,7 +175,7 @@ export abstract class Icon {
   *
   * @param _group The icon group.
   */
-  protected drawIcon_(_group: Element) {}
+  protected drawIcon_(_group: SVGGElement) {}


do we allow developers to create their own icon classes and pass them to our code? I don't think so, but I'm not 100%

if so then this might be too restrictive if they would otherwise be able to pass e.g. a div with an image tag or something.

Good point. I don't think that we do, but it is technically a breaking change so I'll put it back.

maribethb · 2022-10-27T00:12:35Z

core/mutator.ts

-        }
+      }
+      if (this.iconGroup_) {
+        dom.removeClass(this.iconGroup_, 'blocklyIconGroupReadonly');


seems like we are now always removing the class, but before we were adding it or removing it depending on the new editable state. I don't see where we're ever adding the class, is this change intended?

And this is why we have code reviews! Nope, I misread and thought they were doing the same thing. Good catch.

maribethb · 2022-10-27T00:17:04Z

core/mutator.ts

-  private resizeBubble_() {
+  private resizeBubble() {
+    // If the bubble exists, the workspace also exists.
+    const ws = this.workspace as WorkspaceSvg;


I like using the ! instead of casting, the intention is more clear (with the cast, i scrolled back up to see if this was allowed to be a plain Workspace to understand why we needed the cast). does it work to const ws = this.workspace!;?

Alternatively I think if (!this.workspace) return; is even better since it's type safer and about the same amount of code. you could do the same below in setVisible to eliminate more non-null assertions

Done and done.

maribethb · 2022-10-27T00:20:40Z

core/mutator.ts

-      this.rootBlock_ = block.decompose!(this.workspace_!)!;
-      const blocks = this.rootBlock_!.getDescendants(false);
+      this.rootBlock = block.decompose!(this.workspace!)!;
+      const blocks = this.rootBlock!.getDescendants(false);


you can eliminate the ! here since you've already asserted above

maribethb · 2022-10-27T00:32:32Z

core/mutator.ts

@@ -456,7 +454,7 @@ export class Mutator extends Icon {
      block.rendered = false;

      // Allow the source block to rebuild itself.
-      block.compose!(this.rootBlock_);
+      block.compose!(this.rootBlock);


random aside: pretty sure we could have block say it is a type that describes a "Block where we know the compose property is present" since we know it actually must be present if we are calling this, using union types. that would be fancy and i'm only mentioning it here because i'm excited by how specific we can get with TS, not because i think we should actually change anything rn

Yes! I'm not doing it now but I agree that there should be a way to communicate that to the compiler and it would be fancy.

maribethb · 2022-10-27T00:38:54Z

core/warning.ts

@@ -143,8 +143,8 @@ export class Warning extends Icon {
   */
  getText(): string {
    const allWarnings = [];
-    for (const id in this.text_) {
-      allWarnings.push(this.text_[id]);
+    for (const id in this.text) {


is this for loop different than this.text.values()? we probably just didn't use it before because of IE? (i know you didn't add this but just wondering)

I think that this.text should probably become a Map because it's all internal. @gonfunko was there a specific reason you skipped this when you were updating to Maps?

rachel-fenichel

Done, and I also updated tests to use helpers for assertions.

rachel-fenichel · 2022-10-27T21:11:21Z

core/icon.ts

@@ -175,7 +175,7 @@ export abstract class Icon {
   *
   * @param _group The icon group.
   */
-  protected drawIcon_(_group: Element) {}
+  protected drawIcon_(_group: SVGGElement) {}


Good point. I don't think that we do, but it is technically a breaking change so I'll put it back.

rachel-fenichel · 2022-10-27T21:18:07Z

core/mutator.ts

-  private resizeBubble_() {
+  private resizeBubble() {
+    // If the bubble exists, the workspace also exists.
+    const ws = this.workspace as WorkspaceSvg;


Done and done.

rachel-fenichel · 2022-10-27T21:19:14Z

core/mutator.ts

-      this.rootBlock_ = block.decompose!(this.workspace_!)!;
-      const blocks = this.rootBlock_!.getDescendants(false);
+      this.rootBlock = block.decompose!(this.workspace!)!;
+      const blocks = this.rootBlock!.getDescendants(false);


rachel-fenichel · 2022-10-27T21:20:15Z

core/mutator.ts

@@ -456,7 +454,7 @@ export class Mutator extends Icon {
      block.rendered = false;

      // Allow the source block to rebuild itself.
-      block.compose!(this.rootBlock_);
+      block.compose!(this.rootBlock);


Yes! I'm not doing it now but I agree that there should be a way to communicate that to the compiler and it would be fancy.

rachel-fenichel · 2022-10-27T21:21:43Z

core/warning.ts

@@ -143,8 +143,8 @@ export class Warning extends Icon {
   */
  getText(): string {
    const allWarnings = [];
-    for (const id in this.text_) {
-      allWarnings.push(this.text_[id]);
+    for (const id in this.text) {


I think that this.text should probably become a Map because it's all internal. @gonfunko was there a specific reason you skipped this when you were updating to Maps?

rachel-fenichel added 3 commits October 25, 2022 14:43

chore: remove underscores from properties and methods in comment.ts

86ef799

chore: remove some underscores from icon-related classes

43198da

chore: cleanup

a60dd5d

rachel-fenichel requested a review from a team as a code owner October 26, 2022 22:28

rachel-fenichel requested a review from maribethb October 26, 2022 22:28

github-actions bot added the PR: chore General chores (dependencies, typos, etc) label Oct 26, 2022

github-actions bot assigned maribethb Oct 26, 2022

maribethb reviewed Oct 27, 2022

View reviewed changes

chore: respond to PR feedback

52a62e9

rachel-fenichel commented Oct 27, 2022

View reviewed changes

chore: catch one more bit of PR feedback

f2d6504

maribethb approved these changes Oct 31, 2022

View reviewed changes

rachel-fenichel merged commit f95af36 into google:develop Oct 31, 2022

rachel-fenichel deleted the comment_cleanup branch October 31, 2022 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: remove some underscores in icon-related classes #6583

chore: remove some underscores in icon-related classes #6583

rachel-fenichel commented Oct 26, 2022

maribethb Oct 26, 2022

rachel-fenichel Oct 27, 2022

maribethb Oct 26, 2022

rachel-fenichel Oct 27, 2022

maribethb Oct 27, 2022

rachel-fenichel Oct 27, 2022

maribethb Oct 27, 2022

rachel-fenichel Oct 27, 2022

maribethb Oct 27, 2022

rachel-fenichel Oct 27, 2022

maribethb Oct 27, 2022

rachel-fenichel Oct 27, 2022

maribethb Oct 27, 2022

rachel-fenichel Oct 27, 2022

rachel-fenichel left a comment

rachel-fenichel Oct 27, 2022

rachel-fenichel Oct 27, 2022

rachel-fenichel Oct 27, 2022

rachel-fenichel Oct 27, 2022

rachel-fenichel Oct 27, 2022

chore: remove some underscores in icon-related classes #6583

chore: remove some underscores in icon-related classes #6583

Conversation

rachel-fenichel commented Oct 26, 2022

The basics

The details

Resolves

Proposed Changes

Behavior Before Change

Behavior After Change

Reason for Changes

Test Coverage

Documentation

Additional Information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rachel-fenichel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment