-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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][Docs] Merge Canvas function references #38300
Conversation
d682b0b
to
b08691d
Compare
Pinging @elastic/kibana-docs |
Pinging @elastic/kibana-canvas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM. I like it!
2b8c14e
to
a9178a3
Compare
a9178a3
to
6eda8b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting on the correct PR this time!
2ed90a1
to
d5f1c39
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another batch of reviews.
How are we keeping this file in sync with the help strings we're using in the UI? Are we? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the last of my initial edits. Let me know if you have any questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So many excellent changes here. Accepting to unblock, but please review my comments before committing.
examples: Object.values(FontWeight).join(', '), | ||
list: Object.values(FontWeight) | ||
.slice(0, -1) | ||
.map(weight => `\`"${weight}"\``) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would argue that "
and code ticks are a bit redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a formatting decision, I chose to wrap any argument values in back ticks so this show up as inline code. Again related to other comment about markdown, it will render correctly in the docs when they're published, but they will look kinda funky in autocomplete until we can add support for markdown syntax, which I'm hoping I can tackle in the next release (I'm assuming it's a counts as a feature).
@@ -37,7 +37,6 @@ export function lt(): ExpressionFunction<'lt', Context, Arguments, boolean> { | |||
return false; | |||
} | |||
|
|||
// @ts-ignore #35433 This is a wonky comparison for nulls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I fixed this in #37305, but I didn't realize there was an issue to close. My fault for skimming over the comment. I removed boolean
and null
as valid context
types for gt
, gte
, lt
, and lte
. The comment was left behind, so I'm deleting it here, but the change to the function def was merged already.
defaultMessage: 'Return if the context is greater than or equal to the argument', | ||
defaultMessage: 'Returns whether the {context} is greater or equal to the argument.', | ||
values: { | ||
context: '_context_', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: are these underscores converted with markdown somehow? Or should there be code tick marks in the default message instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently autocomplete in Canvas doesn't support markdown, but I filed an issue a while back to add support for markdown rendering. #38439
This is the same syntax used in asciidoc, so it will show up as italicized in the published docs. For the sake of keeping all the content in sync including text formatting, I kept the syntax in even though you see the raw characters in the autocomplete.
x-pack/legacy/plugins/canvas/canvas_plugin_src/strings/functions/pie.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/canvas/canvas_plugin_src/strings/functions/repeatImage.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/canvas/canvas_plugin_src/strings/functions/seriesStyle.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/canvas/canvas_plugin_src/strings/functions/seriesStyle.ts
Outdated
Show resolved
Hide resolved
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
* 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
* 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
* [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)
* [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
Summary
Blocked by #37614.All changes from #37614 are reflected in this PR.It's no longer useful to distinguish between common, client, and server from the user's standpoint since only server functions can run on the server while all other functions run on the client without any way of switching between the two.
I've combined all of the functions into a single list of all functions in Canvas.
This PR also covers changes to the function copy/i18n strings.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] This was checked for cross-browser compatibility, including a check against IE11- [ ] Unit or functional tests were updated or added to match the most common scenarios- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers