blockly-samples icon indicating copy to clipboard operation
blockly-samples copied to clipboard

Replace Closure date picker with HTML date input in field-date

Open rachel-fenichel opened this issue 3 years ago • 2 comments

Category

  • Plugins

Component

field-date

Is your feature request related to a problem? Please describe.

The date field plugin uses the Closure date picker. This adds significant overhead in compilation and size.

Describe the solution you'd like

Update the field-date plugin to use a standard HTML date input.

Describe alternatives you've considered

Deprecate this plugin entirely and write a different one that uses the default date input. Cons: extra upgrade cost. Pros: no silent changes in date picker behaviour. After discussion with @cpcallen, my preference is to update the plugin as a major change.

Additional context

Found while working on https://github.com/google/blockly-samples/pull/1295

The date picker is the last remnant of the Closure UI library that we use directly. It's not in the core library in part because of how large it is. Removing IE support means we don't need to cling to it anymore.

rachel-fenichel avatar Sep 23 '22 20:09 rachel-fenichel

@NeilFraser in case you have additional context or comments.

rachel-fenichel avatar Sep 23 '22 20:09 rachel-fenichel

related to #1168 and could probably be combined. this is specifically about closure UI, the original is about the use of closure in general. a rewrite should be in vanilla js or ts and not use cc

maribethb avatar Sep 24 '22 20:09 maribethb

Hey! I'm a uWaterloo Software Engineering student who's interested in trying my hands at this issue.

jz-yan avatar Nov 17 '22 23:11 jz-yan

@jzyan1999: Happy to assign this to you, at least tentatively: sometimes my colleagues have more insight into particular issues and suggest it would be better for one of us to tackle.

Some quick thoughts about approach:

  1. The first and most important goal is to get rid of all of the lines at the top of plugins/field-date/src/field_date.js that goog.require any goog.* module. (The goog.require of Blockly.* is fine, since these can easily be refactored as imports later.)
    • The main task will be to replace the use of goog.ui.DatePicker with a . This should be reasonably straight forward and hopefully simplify the code.
    • There is going to entail at least one breaking change of the module, however: the .value and .DEFAULT_VALUE properties of FieldDate objects are currently goog.date.Date instances; these will have to be replaced by a different class, most probably just plain old Date objects.
      • Beware, however: since JavaScript Date objects also have a time value associated with them, a given Date object might render as different calendar dates in different timezones. You will need to think about how this will work and make sure it is clearly documented—but it is probably fine just to say that the Date object returned will be (say) 00:00 (or 12:00) of the given date in the user's local timezone.
  2. Next, the refactoring into an ES or TS module will allow the goog.require statements to be replaced by imports, and in conjunction with this the multiple requires of separate internal pieces of Blockly can be replaced with a simple import * as Blockly from 'blockly';, thereby using the Blockly public API.
  3. Then the use of Closure Compiler to build the module can then be eliminated. This module will hopefully be simple enough that (other than tsc if using TypeScript) it may not really require a build step at all.
    • Note that if the shipped NPM contains class definitions then there is another breaking change since anyone subclassing FieldDate using ES5-style subclassing will probably find their code throws an error due to trying to call the FieldDate constructor without new. (In practice this is unlikely to cause much difficulty for most external devs, but will need to be noted in PR description and release notes.)

Note that pieces 1, 2, and 3 are are each useful on their own, and that—while it will be easiest to do them in that order—strictly speaking only piece 1 needs to be finished to satisfy this bug (pieces 2 and 3 are more for #1163, which I will be happy to assign to you if you finish piece 1 and are interested in doing a bit more).

cpcallen avatar Nov 18 '22 16:11 cpcallen

Hey @jzyan1999: are you still planning to send us a PR for this? If not I will unassign this issue (though of course you are still welcome to send us a PR or request it be re-assigned to you).

cpcallen avatar Feb 28 '23 18:02 cpcallen

@btw17 is working on this

maribethb avatar Jul 05 '23 21:07 maribethb

I believe this is complete! So I'm going to close.

BeksOmega avatar Jul 14 '23 00:07 BeksOmega