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

feat!: updated plugins/field-date to TypeScript #1705

Merged
merged 7 commits into from
Jul 7, 2023

Conversation

btw17
Copy link
Member

@btw17 btw17 commented May 22, 2023

Notes

  • Recreated the Date plugin from scratch using the plugin creation dev script
  • Updated to use the browser's date input rather than a text-input with a custom date picker
    • The goal was to deprecate the previous goog.require imports, which included the custom date picker
  • Updated package-lock.json files after npm run clean:node and npm run boot at the root folder.
    • It updates the lock to include blockly: ^10.0.0, matching their corresponding blockly declarations that are already set to that version.

Changes

  1. Uses TypeScript now

Breaking Changes

  1. Can no longer customize date picker colors
    1. There are some styles that are available, such as ::-webkit-calendar-picker-indicator, though they're inconsistent across browsers.
  2. Format was updated from ISO (yyyy-mm-dd) to the browser default for date input.
    1. The ends up being the locale region in most cases, though this can differ depending on system settings and browser.
    2. As a result of this, there can be a desync between the format of the value displayed in the block vs the value in the input when clicking on the block
  3. Updated the config options:
    1. The tooltip config option is now passed to parent components, so it works instead of being ignored!
    2. Set spellcheck?: never since it isn't used for FieldDate
    3. Removed textEdit option. Previously, this allowed using the Date Picker while disabling the text input. This cannot be done with the default browser input, as disabling the input disables the picker associated with it.

@btw17 btw17 force-pushed the feat/field-date-ts branch 2 times, most recently from 650a384 to 239ade4 Compare May 27, 2023 00:06
@btw17 btw17 force-pushed the feat/field-date-ts branch 5 times, most recently from e5a71f2 to 86324dc Compare July 5, 2023 18:01
@btw17 btw17 marked this pull request as ready for review July 5, 2023 18:04
@btw17 btw17 requested a review from a team as a code owner July 5, 2023 18:04
@btw17 btw17 requested review from BeksOmega and removed request for a team July 5, 2023 18:04
Copy link
Contributor

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

So excited to remove the last last vestige of closure library :D

plugins/field-date/src/field_date.ts Show resolved Hide resolved
plugins/field-date/src/field_date.ts Outdated Show resolved Hide resolved
plugins/field-date/src/field_date.ts Show resolved Hide resolved
@@ -97,7 +119,7 @@ suite('FieldDate', function() {
});
test('New Value', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know all of the validater tests already existed, but could you update them to have some more informative names? Not a high priority though if you just want to get this merged!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@@ -49,8 +51,7 @@ Blockly.defineBlocksWithJsonArray([
}]);
```

[View the developer documentation](https://developers.google.com/blockly/guides/create-custom-blocks/fields/built-in-fields/date) for further usage examples.
- [View the developer documentation](https://developers.google.com/blockly/guides/create-custom-blocks/fields/built-in-fields/date) for further usage examples.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this still conform to the documented API? Even after your changes related to date parsing? My reading is that it does, but wanted to check!

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like the link is no longer available, actually. I'm assuming this was originally in the core repo, though since it's in plugins now it's no longer documented there.

I went ahead and removed this link.

plugins/field-date/package.json Show resolved Hide resolved
protected showEditor_(e?: Event, _quietInput?: boolean) {
if (this.sourceBlock_) {
// NOTE: Disable modal inputs to use default browser inputs on mobile.
this.sourceBlock_.workspace.options.modalInputs = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that we want to disable modalinputs for the whole workspace to get the date picker to work. I would rather:

  1. Pass quietInput as true to super, but focus on it anyway.
  2. Tell external devs to disable modal inputs in their workspace config if they want to use this plugin. We could also log a warning from here if we run into enabled modalInputs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I went ahead and implemented option 1 of your suggestions. It seemed the least invasive. TBH, I'm not in tune with the use case of needing to set quietInput + avoid focusing on the input, though, so I'm not sure how important that is.

plugins/field-date/src/field_date.ts Show resolved Hide resolved
@btw17 btw17 changed the title feat: updated plugins/field-date to TypeScript feat!: updated plugins/field-date to TypeScript Jul 7, 2023

const tooltipConfig: FieldDateFromJsonConfig = {
date: '2021-03-13',
tooltip: 'This date block has a tooltip!',
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding a new test block for this =)

@BeksOmega BeksOmega merged commit e5531ff into google:master Jul 7, 2023
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