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

chore: remove underscores and clean up scrollbar.ts #6584

Merged
merged 4 commits into from Oct 28, 2022

Conversation

rachel-fenichel
Copy link
Collaborator

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

In three commits:

  • Restructure the constructor so that properties are set in the constructor, not in a helper function. This let me remove nullability from several SVGElements that the scrollbar tracks.
  • Stop tracking an SVG Element that was only used during construction.
  • Change dispose to remove the scrollbar's root node but not set anything else to null. This also helped with nullability. The garbage collector is in charge of the rest of cleanup.
  • Add comments to property definitions to make them more meaningful for the next reader.
  • Rename all private properties and methods that end in underscores to remove the underscores.

Behavior Before Change

No change.

Behavior After Change

No change.

Reason for Changes

Cleanup, and reading code to understand dependencies.

Test Coverage

Mocha tests, but I also opened up the playground and both dragged and clicked scrollbars in several situations:

  • LTR, desktop
  • LTR, touch
  • RTL, desktop
  • RTL, touch

Documentation

None

Additional Information

@rachel-fenichel rachel-fenichel requested a review from a team as a code owner October 27, 2022 00:18
@github-actions github-actions bot added the PR: chore General chores (dependencies, typos, etc) label Oct 27, 2022
Comment on lines 204 to 205
// Store the thickness in a temp variable for readability.
const scrollbarThickness = Scrollbar.scrollbarThickness;
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is not very interesting, and probably needn't have been preserved.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed.

/**
* Set the size of the scrollbar DOM elements along the minor axis.
*/
private setInitialThickness() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Opinion: I would probably not have bothered extracting this as a separate method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Extracting it makes the constructor fit in a single window for me without rotating my monitor :)

I actually would rather have extracted the lengthAttribute/positionAttribute setting as well, but doing it directly in the constructor means that the compiler is certain about nullability.

Comment on lines 237 to 240
if (this.svgHandle_) {
this.workspace.getThemeManager().unsubscribe(this.svgHandle_);
}
this.workspace.getThemeManager().unsubscribe(this.svgHandle_);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Aside: how did this not fail our format check??

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, huh. It looks fine here, but in the side-by-side diff it looked like it was previously:

   if (this.svgHandle_) {
    this.workspace.getThemeManager().unsubscribe(this.svgHandle_);
  }
  }

@rachel-fenichel rachel-fenichel merged commit 25c87c1 into google:develop Oct 28, 2022
@rachel-fenichel rachel-fenichel deleted the cleanup_scrollbars branch October 28, 2022 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: chore General chores (dependencies, typos, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants