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

Updating bump logic to support single-direction scrollbars #4652

Merged
merged 10 commits into from
Mar 1, 2021

Conversation

moniika
Copy link
Contributor

@moniika moniika commented Feb 24, 2021

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide

The details

Resolves

Part of #1292

Proposed Changes

  • Metrics changes:
    • Adds scroll metrics. (getScrollMetrics and adds scroll metrics to the metrics object Blockly.utils.Metrics)
    • Changes content metrics computation/usage
  • Bump logic (bumping blocks/workspace comments within fixed edges of workspace):
    • Bumps objects if they are created/moved outside of scroll area
    • Bumps top objects on window resize (where scroll area may have changed)

Reason for Changes

Necessary to support single-direction scrollbar

Test Coverage

Extensive manual testing in playground. Mocha tests.

Tested on:

  • Desktop Chrome

Documentation

Additional Information

Remake of #4618

@moniika moniika force-pushed the bump-with-cleanup branch 2 times, most recently from 644cd11 to f8a9a47 Compare February 25, 2021 02:26
core/inject.js Outdated Show resolved Hide resolved
Copy link
Contributor

@alschmiedt alschmiedt left a comment

Choose a reason for hiding this comment

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

Not necessarily in this PR, but I am curious what your thoughts are on pulling out the bump logic from inject? I think this would give us the ability to register the class later on, so someone could create their own bump blocks logic.

core/interfaces/i_metrics_manager.js Outdated Show resolved Hide resolved
core/interfaces/i_metrics_manager.js Outdated Show resolved Hide resolved
core/metrics_manager.js Outdated Show resolved Hide resolved
core/metrics_manager.js Outdated Show resolved Hide resolved
core/metrics_manager.js Outdated Show resolved Hide resolved
core/utils/metrics.js Show resolved Hide resolved
core/events/events.js Outdated Show resolved Hide resolved
core/events/events.js Outdated Show resolved Hide resolved
core/inject.js Show resolved Hide resolved
core/inject.js Outdated Show resolved Hide resolved
Copy link
Contributor

@alschmiedt alschmiedt left a comment

Choose a reason for hiding this comment

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

A few small comments left, but then lgtm!

core/inject.js Outdated Show resolved Hide resolved
core/inject.js Outdated Show resolved Hide resolved
core/metrics_manager.js Outdated Show resolved Hide resolved
core/metrics_manager.js Outdated Show resolved Hide resolved
core/inject.js Outdated
@@ -387,7 +384,7 @@ Blockly.init_ = function(mainWorkspace) {
mainWorkspace, true, true, 'blocklyMainWorkspaceScrollbar');
mainWorkspace.scrollbar.resize();
} else {
mainWorkspace.setMetrics({x: 0.5, y: 0.5});
mainWorkspace.setMetrics({x: 0, y: 0});
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why the change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I changed it because it fixed unit tests. However it looks like it doesn't make a difference now, so I'll change it back.
I think it's possible that the scrollbar logic refactor PR may have something to do with it.

@moniika moniika merged commit 57749e6 into google:develop Mar 1, 2021
@moniika moniika deleted the bump-with-cleanup branch March 1, 2021 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants