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.

Sign up for GitHub

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

Jump to bottom

Removing (most) Closure from BlockFactory #1925

Merged
merged 20 commits into from
Jun 15, 2018
Merged

Removing (most) Closure from BlockFactory #1925

merged 20 commits into from
Jun 15, 2018

Conversation

AnmAtAnm
Copy link
Contributor

@AnmAtAnm AnmAtAnm commented Jun 15, 2018 •
edited
Loading

The basics

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

The details

Resolves

#668 (partially)

Proposed Changes

Replacing all Closure goog. library calls from BlockFactory, with the exception of the ColorPicker and the test for the warning about running without Closure. This includes (almost) all goog.provide and goog.require statements.

Final status:

$ grep -rF "goog." .
./index.html:      Add it only if you need it.  See also goog.require in blockly.js.
./app_controller.js:goog.require('goog.dom.xml');  // Used to detect Closure
./app_controller.js:  if (!window.goog.dom.xml) {
./workspacefactory/wfactory_init.js:goog.require('goog.ui.PopupColorPicker');
./workspacefactory/wfactory_init.js:goog.require('goog.ui.ColorPicker');
./workspacefactory/wfactory_init.js:  var colourPicker = new goog.ui.ColorPicker();
./workspacefactory/wfactory_init.js:  var popupPicker = new goog.ui.PopupColorPicker(null, colourPicker);
./workspacefactory/wfactory_init.js:  goog.events.listen(popupPicker, 'change', function(e) {

I'll remove the ColorPicker and warning in a follow-up PR.

Reason for Changes

Recent changes in Closure have been breaking BlockFactory, which uses Closure libraries but was not compiled by the Closure Compiler. This also simplifies use for people host their own BlockFactory, or running the local file from the repo.

Test Coverage

Every changed function and class was run at least once (manual testing), verifying I did not break its primary behavior. Most changes are trivial.

Tested on:

  • Desktop Chrome

@AnmAtAnm
Removing goog library call from block_exporter_tools.js.
bee387d 
Removing unused method generateToolboxFromLibrary().
@AnmAtAnm
Removing goog.provide/require('BlockExporterTools')
fa2ebdb
@AnmAtAnm
Removing goog.provide/require('BlocklyDevTools.Analytics')
2790f8e
@AnmAtAnm
Removing goog. (Closure) library calls from block_definition_extractor
d7b418c
@AnmAtAnm
Removing goog.provide/require('BlockDefinitionExtractor')
412bc46
@AnmAtAnm
Removing Closure library goog call from factory_utils.js
81c3641
@AnmAtAnm
Removing goog.provide/require('StandardCategories')
a3a701f
@AnmAtAnm
Removing goog.provide/require('FactoryUtils')
bf12e66
@AnmAtAnm
Removing Closure library goog calls from wfactory_generator.js
44b97bd
@AnmAtAnm
Removing Closure library goog. calls from block_option.js
4fc83f2
@AnmAtAnm
Removing goog.provide/require('BlockOption')
66c4fe0
@AnmAtAnm
Removing goog.provide/require('BlockFactory')
1817ea1
@AnmAtAnm
Removing Closure library goo. calls from block_library_view.js
1a1228f
@AnmAtAnm
Removing goog.provide/require('BlockLibraryView')
5d6e247
@AnmAtAnm
Removing goog.provide/require('BlockLibraryStorage')
01578ff
@AnmAtAnm
Removing Closure from block_library_controller.js
e8c50a7 
Removing goog.require('BlockLibraryController') elsewhere.
@AnmAtAnm
Removing goog.provide/require('BlockExporterView')
e443970
@AnmAtAnm
Removing goog.provide/require('BlockExporterController')
7d19f2f
@AnmAtAnm AnmAtAnm requested a review from picklesrus June 15, 2018 01:02
picklesrus
picklesrus reviewed Jun 15, 2018
View reviewed changes
demos/blockfactory/block_definition_extractor.js
BlockDefinitionExtractor.newDomElement_('value', {name: 'COLOUR'});
colourInputValue.append(colourBlock);
factoryBaseEl.append(colourInputValue);
var colour_hue = block.getHue(); // May be null if not set via hue.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these TODOs actually done now?

Copy link
Contributor Author

@AnmAtAnm AnmAtAnm Jun 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

#1247 was solved by #1293, which added the .getHue() method.

demos/blockfactory/block_exporter_tools.js
* @param {!BlockLibraryStorage} blockLibStorage Block Library Storage object.
* @return {!Element} XML representation of the toolbox.
*/
BlockExporterTools.prototype.generateToolboxFromLibrary
Copy link
Contributor

Choose a reason for hiding this comment

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

was this unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

demos/blockfactory/block_library_storage.js
// goog.global is synonymous to window, and allows for flexibility
// between browsers.
var object = goog.global.localStorage[this.name];
var object = localStorage[this.name];
Copy link
Contributor

Choose a reason for hiding this comment

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

none of the browsers require this to be window.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

window is the global scope in a browser context: https://stackoverflow.com/a/12660102/152543

demos/blockfactory/block_library_view.js Outdated
var option = document.createElement('a');
option.id ='dropdown_' + blockType;
option.classList.add('blockLibOpt');
option.innerHTML = blockType;
Copy link
Contributor

Choose a reason for hiding this comment

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

we try and avoid innerHtml. What is this? can it be option.textNode = blockType (or whatever it actually is)

demos/blockfactory/block_option.js Outdated
}, this.blockType);
var labelText = document.createElement('p');
labelText.id = this.blockType + '_text';
labelText.innerHTML = this.blockType;
Copy link
Contributor

Choose a reason for hiding this comment

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

same innerHtml comment here.

AnmAtAnm
AnmAtAnm commented Jun 15, 2018
View reviewed changes
Copy link
Contributor Author

@AnmAtAnm AnmAtAnm left a comment

Choose a reason for hiding this comment

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

Replaced all .innerHTML assignments with .textContent.

demos/blockfactory/block_definition_extractor.js
BlockDefinitionExtractor.newDomElement_('value', {name: 'COLOUR'});
colourInputValue.append(colourBlock);
factoryBaseEl.append(colourInputValue);
var colour_hue = block.getHue(); // May be null if not set via hue.
Copy link
Contributor Author

@AnmAtAnm AnmAtAnm Jun 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

#1247 was solved by #1293, which added the .getHue() method.

demos/blockfactory/block_exporter_tools.js
* @param {!BlockLibraryStorage} blockLibStorage Block Library Storage object.
* @return {!Element} XML representation of the toolbox.
*/
BlockExporterTools.prototype.generateToolboxFromLibrary
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

demos/blockfactory/block_library_storage.js
// goog.global is synonymous to window, and allows for flexibility
// between browsers.
var object = goog.global.localStorage[this.name];
var object = localStorage[this.name];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

window is the global scope in a browser context: https://stackoverflow.com/a/12660102/152543

picklesrus
picklesrus reviewed Jun 15, 2018
View reviewed changes
demos/blockfactory/color_picker.js Outdated
*/

/**
* @fileoverview Color picker used to select the color of a toolbox category.
Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean to add this file in this commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ummm... what file? 👀

(removed)

@picklesrus
Copy link
Contributor

LGTM

@AnmAtAnm AnmAtAnm merged commit b4cfe26 into google:develop Jun 15, 2018
@AnmAtAnm AnmAtAnm deleted the no-goog-devtools branch June 18, 2018 18:46
@rachel-fenichel rachel-fenichel mentioned this pull request Jun 26, 2018
Closed
@rachel-fenichel rachel-fenichel mentioned this pull request Jun 26, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@picklesrus picklesrus picklesrus left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AnmAtAnm @picklesrus