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

[Canvas] Clean up expression function definitions #37305

Merged
merged 10 commits into from
Jun 7, 2019

Conversation

cqliu1
Copy link
Contributor

@cqliu1 cqliu1 commented May 29, 2019

Summary

Related to #37614.

Updating all functions definitions to be more accurate/uniform. The changes in #37614 should reflect any updates made in this PR, so it'd be good to review both side-by-side. I tried my best change as little functionality as possible.

Changes:

  • added content as an alias for the unnamed arg for markdown
  • flags required arguments as required: true
  • explicitly defined context type as null for functions that don't use context or pass it along
  • fix default values
  • removed null as valid context/arg type where applicable
  • added fn, exp, function, expression to all args that had a subset of these aliases to be consistent
  • removed boolean and null as context types and boolean as value type for gt, gte, lt, and lte because these operations only make sense for numbers and sometimes strings
  • removed obsolete browser and server functions since the interpreter now runs every function in the browser unless it's a server function

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11

For maintainers

@cqliu1 cqliu1 force-pushed the chore/fn-def-clean-up branch 4 times, most recently from 68f3ccf to 096d145 Compare May 29, 2019 19:42
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cqliu1 cqliu1 added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label May 31, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas

@cqliu1 cqliu1 added chore release_note:skip Skip the PR/issue when compiling release notes v7.3.0 v8.0.0 labels May 31, 2019
@cqliu1 cqliu1 force-pushed the chore/fn-def-clean-up branch from 8361fb4 to effaf61 Compare May 31, 2019 09:19
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kibanamachine kibanamachine mentioned this pull request May 31, 2019
21 tasks
@cqliu1 cqliu1 force-pushed the chore/fn-def-clean-up branch 2 times, most recently from 761a8b7 to ead8fdf Compare June 4, 2019 22:35
@cqliu1 cqliu1 requested review from w33ble and crob611 June 4, 2019 22:35
@cqliu1 cqliu1 added the review label Jun 4, 2019
@cqliu1 cqliu1 marked this pull request as ready for review June 4, 2019 22:35
@cqliu1 cqliu1 requested a review from a team as a code owner June 4, 2019 22:35
@cqliu1 cqliu1 added release_note:deprecation and removed release_note:skip Skip the PR/issue when compiling release notes labels Jun 4, 2019
Copy link
Contributor

@w33ble w33ble left a comment

Choose a reason for hiding this comment

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

The only change I'd say is needed here is the required setting on sort.by, the rest is mostly questions.

@@ -63,14 +63,14 @@ export function font(): NullContextFunction<'font', Arguments, Style> {
types: ['boolean'],
},
lHeight: {
aliases: ['lineHeight'],
aliases: ['lineHeight', 'null'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I think null was supposed to be added to types instead of aliases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! I meant to put to it on types. I'll fix it.

@cqliu1 cqliu1 force-pushed the chore/fn-def-clean-up branch 3 times, most recently from 65bd457 to 125d17c Compare June 6, 2019 17:10
cqliu1 added a commit to cqliu1/kibana that referenced this pull request Jun 6, 2019
@cqliu1 cqliu1 force-pushed the chore/fn-def-clean-up branch from 6757897 to 91f010d Compare June 7, 2019 17:34
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cqliu1 cqliu1 merged commit 3c9182f into elastic:master Jun 7, 2019
@cqliu1 cqliu1 deleted the chore/fn-def-clean-up branch June 7, 2019 21:32
cqliu1 added a commit to cqliu1/kibana that referenced this pull request Jun 10, 2019
        * Cleaned up common function defs

            fixed broken tests

            Remove boolean and null as context/arg valid types in lt, lte, gt, and gte

            Added 'function' alias

            Updated comparator tests

            Added neq to options in compare function

            null audit

        * Deleted obsolete 'browser' and 'server' functions

        * More clean up

        * Removed server and browser function i18n strings

        * Fixed broken tests

        * Addressing feedback

        * Removed browser and server from function_help

        * Added ts-ignore for constant import

        * Fixed help text label

        * Fixed font
cqliu1 added a commit that referenced this pull request Jun 10, 2019
* Cleaned up common function defs

            fixed broken tests

            Remove boolean and null as context/arg valid types in lt, lte, gt, and gte

            Added 'function' alias

            Updated comparator tests

            Added neq to options in compare function

            null audit

        * Deleted obsolete 'browser' and 'server' functions

        * More clean up

        * Removed server and browser function i18n strings

        * Fixed broken tests

        * Addressing feedback

        * Removed browser and server from function_help

        * Added ts-ignore for constant import

        * Fixed help text label

        * Fixed font
cqliu1 added a commit to cqliu1/kibana that referenced this pull request Jun 20, 2019
cqliu1 added a commit to cqliu1/kibana that referenced this pull request Jul 11, 2019
* Synced up docs with function defs

* Updated docs with changes from PR elastic#37305

* Merged canvas function ref into a single doc

* Fixed arg order

* Fixed typo

* Added alphabet links

* Added alphabet headers

* Removed B header

* Added missing args from

* Added edits from PR elastic#37614

* Updated containerStyle

* Removed metafields. ESSQL doesn't support retrieving metafields

* Edits to function copy

* More edits

* More edits

* Final round of edits

* Fixed i18n errors

* Addressing feedback

* Fixed jest test

* Fixed missing import

* Fixed i18n error

* Restored metaFields arg in esdocs

* Extracted i18n string constants

* Fixed i18n errors

* Updated translation files
cqliu1 added a commit that referenced this pull request Jul 11, 2019
* Synced up docs with function defs

* Updated docs with changes from PR #37305

* Merged canvas function ref into a single doc

* Fixed arg order

* Fixed typo

* Added alphabet links

* Added alphabet headers

* Removed B header

* Added missing args from

* Added edits from PR #37614

* Updated containerStyle

* Removed metafields. ESSQL doesn't support retrieving metafields

* Edits to function copy

* More edits

* More edits

* Final round of edits

* Fixed i18n errors

* Addressing feedback

* Fixed jest test

* Fixed missing import

* Fixed i18n error

* Restored metaFields arg in esdocs

* Extracted i18n string constants

* Fixed i18n errors

* Updated translation files
cqliu1 added a commit to cqliu1/kibana that referenced this pull request Jul 11, 2019
    * Synced up docs with function defs

    * Updated docs with changes from PR elastic#37305

    * Merged canvas function ref into a single doc

    * Fixed arg order

    * Fixed typo

    * Added alphabet links

    * Added alphabet headers

    * Removed B header

    * Added missing args from

    * Added edits from PR elastic#37614

    * Updated containerStyle

    * Removed metafields. ESSQL doesn't support retrieving metafields

    * Edits to function copy

    * More edits

    * More edits

    * Final round of edits

    * Fixed i18n errors

    * Addressing feedback

    * Fixed jest test

    * Fixed missing import

    * Fixed i18n error

    * Restored metaFields arg in esdocs

    * Extracted i18n string constants

    * Fixed i18n errors

    * Updated translation files
cqliu1 added a commit that referenced this pull request Jul 12, 2019
* [Canvas][Docs] Merge Canvas function references (#38300)

* Synced up docs with function defs

* Updated docs with changes from PR #37305

* Merged canvas function ref into a single doc

* Fixed arg order

* Fixed typo

* Added alphabet links

* Added alphabet headers

* Removed B header

* Added missing args from

* Added edits from PR #37614

* Updated containerStyle

* Removed metafields. ESSQL doesn't support retrieving metafields

* Edits to function copy

* More edits

* More edits

* Final round of edits

* Fixed i18n errors

* Addressing feedback

* Fixed jest test

* Fixed missing import

* Fixed i18n error

* Restored metaFields arg in esdocs

* Extracted i18n string constants

* Fixed i18n errors

* Updated translation files

* Fixes Canvas doc headers (#40921)
cqliu1 added a commit that referenced this pull request Jul 12, 2019
* [Canvas][Docs] Merge Canvas function references (#38300)

    * Synced up docs with function defs

    * Updated docs with changes from PR #37305

    * Merged canvas function ref into a single doc

    * Fixed arg order

    * Fixed typo

    * Added alphabet links

    * Added alphabet headers

    * Removed B header

    * Added missing args from

    * Added edits from PR #37614

    * Updated containerStyle

    * Removed metafields. ESSQL doesn't support retrieving metafields

    * Edits to function copy

    * More edits

    * More edits

    * Final round of edits

    * Fixed i18n errors

    * Addressing feedback

    * Fixed jest test

    * Fixed missing import

    * Fixed i18n error

    * Restored metaFields arg in esdocs

    * Extracted i18n string constants

    * Fixed i18n errors

    * Updated translation files

* Fixes Canvas doc headers (#40921)

* Fixed imports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:large Large Level of Effort release_note:deprecation review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.3.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants