moving canvas font function to OSS #37619

ppisljar · 2019-05-31T07:52:55Z

Summary

Moves canvas font function to OSS, along with:

function_wrapper test utility, that its tests require
lib/fonts.ts which is required
types/style.ts which is required
all of the three are reexported in their original place

blocker for #34532

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~~
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~[ ] This was checked for breaking API changes and was labeled appropriately~~
~~[ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-05-31T07:53:48Z

Pinging @elastic/kibana-app-arch

elasticmachine · 2019-05-31T08:34:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-31T09:49:34Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lukeelmers · 2019-06-03T03:44:29Z

src/legacy/core_plugins/interpreter/__tests__/helpers/function_wrapper.js

+
+  return (context, args, handlers) =>
+    spec.fn(context, { ...defaultArgs, ...args }, handlers, mockReduxStore);
+};


Just FYI I think I already copied a (slightly modified) version of this to interpreter awhile back: src/legacy/core_plugins/interpreter/test_helpers.js

elasticmachine · 2019-06-03T04:44:30Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-04T06:53:13Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

ppisljar · 2019-06-06T06:47:54Z

retest

elasticmachine · 2019-06-06T07:46:11Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

clintandrewhall

You can test all of these changes easily in Storybook:

canvas: yarn storybook -> FontPicker

There's a bug currently fixed in #38222 that allows SVGs to load, but that shouldn't prevent you from testing easily.

clintandrewhall · 2019-06-06T07:51:23Z

src/legacy/core_plugins/interpreter/public/functions/help/font.ts

+import { i18n } from '@kbn/i18n';
+import { FontWeight } from '../../types';
+
+export const help: any = {


We should always include a comment explaining why we're putting an any in TS. In this case, it's because the help introspection types haven't been moved up into OSS (yet).

I would add a TODO comment here with a link to an issue (if one exists, otherwise create one) explaining this should be converted to use a similar structure to what exists in Canvas.

This is important. It ensures Expressions are and remain translated.

i moved the translations into the function as we have with the rest of the functions in OSS for now, we'll move them back when/if we add the help introspection to OSS

clintandrewhall · 2019-06-06T07:52:16Z

src/legacy/core_plugins/interpreter/public/types/style.ts

+/**
+ * Collection of text alignments.
+ */
+export const TEXT_ALIGNMENTS: TextAlignment[] = ['center', 'left', 'right', 'justify'];


This should be deleted and callsites converted to TextAlignment.values()... no need to repeat. This was a chore I never cleaned up.

is there a way to convert string literal union to js array ?

is there a way to convert string literal union to js array ?

Yeah IDK if you can do this. I think you can do it the other way though (define a js array and convert it to a string literal union). But I'm not aware of any way to go union > array.

I'm also not sure TextAlignment.values() would work in this case, but @clintandrewhall probably knows of some magic I haven't heard of here

clintandrewhall · 2019-06-06T07:53:28Z

src/legacy/core_plugins/interpreter/public/types/style.ts

+  backgroundColor: string | null;
+  backgroundImage: string | null;
+  backgroundSize: 'contain' | 'cover' | 'auto';
+  backgroundRepeat: 'repeat-x' | 'repeat' | 'space' | 'round' | 'no-repeat' | 'space';


nit: I'd love to see all of these as types, personally.

clintandrewhall · 2019-06-06T07:56:51Z

@lukeelmers Where do we stand on bringing the i18n help and error introspection into OSS? If we're not going to move that upward, that's ok... but I'd insist the font help have a copy or facsimile before being checked in.

elasticmachine · 2019-06-06T09:57:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-06T11:12:26Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-06T12:27:13Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-06T13:31:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lukeelmers

LGTM! Added 2 notes, but they shouldn't block merging this PR; they're intended more for future discussion.

lukeelmers · 2019-06-07T15:33:19Z

src/legacy/core_plugins/interpreter/public/types/style.ts

+/**
+ * Collection of text alignments.
+ */
+export const TEXT_ALIGNMENTS: TextAlignment[] = ['center', 'left', 'right', 'justify'];


is there a way to convert string literal union to js array ?

Yeah IDK if you can do this. I think you can do it the other way though (define a js array and convert it to a string literal union). But I'm not aware of any way to go union > array.

I'm also not sure TextAlignment.values() would work in this case, but @clintandrewhall probably knows of some magic I haven't heard of here

lukeelmers · 2019-06-07T15:45:00Z

src/legacy/core_plugins/interpreter/public/functions/font.ts

+import { i18n } from '@kbn/i18n';
+import { openSans } from '../../common/lib/fonts';
+import { ExpressionFunction } from '../../types';
+import { CSSStyle, FontFamily, FontWeight, Style, TextAlignment, TEXT_ALIGNMENTS } from '../types';


Question on the location (not sure if I know the right answer, just thinking aloud):

Since we know types for fonts are only going to be in /public (not /server), it makes sense that we would export them here. But would it make more sense to colocate them with the other types in interpreter/types, and then simply re-export them from interpreter/public?

It's the first time we've run into this situation, but I suppose it's a question of conventions. Do we want to centralize all types that we know are shared at a top-level (alongside /server and /public), and then export them as needed to either server or public or both? Or do we take items that we know are only ever one or the other and create another types directory inside of public?

Regardless I don't think this is important to solve now -- since this will all be moving to plugins/data eventually we can sort this out at that time. Just thought I'd raise the question to see if you had strong feelings on it.

lukeelmers · 2019-06-07T15:52:52Z

Where do we stand on bringing the i18n help and error introspection into OSS? If we're not going to move that upward, that's ok... but I'd insist the font help have a copy or facsimile before being checked in.

@clintandrewhall For whatever reason I'm having trouble grokking exactly how the help types are working in Canvas, even after looking through the code a couple times. I could probably benefit from a quick sync if you have some time... I think it would be clearer for me to hear someone explain in person, and then would make it easier for us to sort out the best way to apply them here.

clintandrewhall · 2019-06-07T17:33:13Z

@lukeelmers and I chatted today, and I'm going to provide more documentation and a few tweaks to font that will be helpful in a PR. Let's circle back Monday, as well. Thanks!

clintandrewhall · 2019-06-10T15:46:28Z

I created #38554 to help... let me know if I can further clarify!

ppisljar · 2019-06-10T18:16:24Z

thanks @clintandrewhall
regarding the i18n help and error introspection in OSS, i would keep it out of this PR to unblock the effort on improving semantics of OSS functions, and i suggest to tackle this all-at-once for all OSS functions.

elasticmachine · 2019-06-14T09:33:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-14T10:33:59Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

ppisljar · 2019-06-14T11:12:41Z

retest

elasticmachine · 2019-06-14T12:10:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-25T07:09:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ppisljar added the chore label May 31, 2019

ppisljar requested a review from a team as a code owner May 31, 2019 07:52

ppisljar requested a review from w33ble May 31, 2019 07:53

ppisljar added :AppArch v7.3.0 v8.0.0 labels May 31, 2019

lukeelmers force-pushed the move/canvasFontFn branch from f5d75f0 to 7fdfc58 Compare June 3, 2019 03:46

lukeelmers reviewed Jun 3, 2019

View reviewed changes

ppisljar force-pushed the move/canvasFontFn branch from 7fdfc58 to cd9c607 Compare June 4, 2019 05:55

clintandrewhall reviewed Jun 6, 2019

View reviewed changes

ppisljar force-pushed the move/canvasFontFn branch from cd9c607 to 9fcc72c Compare June 6, 2019 08:59

ppisljar force-pushed the move/canvasFontFn branch from 11f2fef to b28e258 Compare June 6, 2019 11:45

lukeelmers approved these changes Jun 7, 2019

View reviewed changes

lukeelmers mentioned this pull request Jun 7, 2019

Move interfaces for interpreter types to OSS #37753

Merged

clintandrewhall mentioned this pull request Jun 10, 2019

[Canvas] Update Type Documentation #38554

Merged

ppisljar added the release_note:skip Skip the PR/issue when compiling release notes label Jun 14, 2019

ppisljar force-pushed the move/canvasFontFn branch from b28e258 to cbb7e8d Compare June 14, 2019 08:39

clintandrewhall approved these changes Jun 24, 2019

View reviewed changes

clintandrewhall removed the request for review from w33ble June 24, 2019 18:01

moving font function to OSS

201a510

ppisljar force-pushed the move/canvasFontFn branch from 3937b98 to 201a510 Compare June 25, 2019 06:05

ppisljar merged commit 01610b1 into elastic:master Jun 25, 2019

ppisljar mentioned this pull request Jun 25, 2019

[7.x] moving font function to OSS (#37619) #39560

Merged

ppisljar added a commit that referenced this pull request Jun 25, 2019

moving font function to OSS (#37619) (#39560)

f1e301a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

moving canvas font function to OSS #37619

moving canvas font function to OSS #37619

ppisljar commented May 31, 2019 •

edited

Loading

elasticmachine commented May 31, 2019

elasticmachine commented May 31, 2019

elasticmachine commented May 31, 2019

lukeelmers Jun 3, 2019

elasticmachine commented Jun 3, 2019

elasticmachine commented Jun 4, 2019

ppisljar commented Jun 6, 2019

elasticmachine commented Jun 6, 2019

clintandrewhall left a comment

clintandrewhall Jun 6, 2019

ppisljar Jun 6, 2019

clintandrewhall Jun 6, 2019

ppisljar Jun 6, 2019

lukeelmers Jun 7, 2019

clintandrewhall Jun 6, 2019

clintandrewhall commented Jun 6, 2019 •

edited

Loading

elasticmachine commented Jun 6, 2019

elasticmachine commented Jun 6, 2019

elasticmachine commented Jun 6, 2019

elasticmachine commented Jun 6, 2019

lukeelmers left a comment

lukeelmers Jun 7, 2019

lukeelmers Jun 7, 2019

lukeelmers commented Jun 7, 2019

clintandrewhall commented Jun 7, 2019

clintandrewhall commented Jun 10, 2019

ppisljar commented Jun 10, 2019

elasticmachine commented Jun 14, 2019

elasticmachine commented Jun 14, 2019

ppisljar commented Jun 14, 2019

elasticmachine commented Jun 14, 2019

elasticmachine commented Jun 25, 2019

moving canvas font function to OSS #37619

moving canvas font function to OSS #37619

Conversation

ppisljar commented May 31, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented May 31, 2019

elasticmachine commented May 31, 2019

💔 Build Failed

elasticmachine commented May 31, 2019

💔 Build Failed

Choose a reason for hiding this comment

elasticmachine commented Jun 3, 2019

💔 Build Failed

elasticmachine commented Jun 4, 2019

💔 Build Failed

ppisljar commented Jun 6, 2019

elasticmachine commented Jun 6, 2019

💔 Build Failed

clintandrewhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintandrewhall commented Jun 6, 2019 • edited Loading

elasticmachine commented Jun 6, 2019

💔 Build Failed

elasticmachine commented Jun 6, 2019

💔 Build Failed

elasticmachine commented Jun 6, 2019

💔 Build Failed

elasticmachine commented Jun 6, 2019

💚 Build Succeeded

lukeelmers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukeelmers commented Jun 7, 2019

clintandrewhall commented Jun 7, 2019

clintandrewhall commented Jun 10, 2019

ppisljar commented Jun 10, 2019

elasticmachine commented Jun 14, 2019

💔 Build Failed

elasticmachine commented Jun 14, 2019

💔 Build Failed

ppisljar commented Jun 14, 2019

elasticmachine commented Jun 14, 2019

💚 Build Succeeded

elasticmachine commented Jun 25, 2019

💚 Build Succeeded

ppisljar commented May 31, 2019 •

edited

Loading

clintandrewhall commented Jun 6, 2019 •

edited

Loading