theme function #73451

ppisljar · 2020-07-28T13:59:29Z

Summary

Adds a theme function as proposed here #71064

Currently theme information (variables) are part of expression variables under theme key. Preferably we introduce a service in the future to provide this information.

How to test:
provide a theme variable to expressions and see your fonts change:

theme variable needs to be sent to executor with the following structure:

theme: {
  font: {
    family: string,
    size: number,
    color: string,
    italics: boolean,
    underline: boolean
    weight: string;
  }
}

every function using font function for any of its arguments where specific font function arguments have not been overridden will not start using the theme information provided

looking at example canvas workpads it seems canvas always sets size family color and alignment when adding a font property to element, which should probably be changed, so it only adds {font} and all the arguments are set only after user changes from whats set on theme.

also, all the example workpads have most of the font arguments set (which means that a theme won't apply).

followup:

theme service would be preferred over using variables
align on palette information, update palette function
updates on canvas side on how the variables/defaults are set thru UI
...

Checklist

Delete any items that are not applicable to this PR.

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
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-07-28T13:59:32Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

flash1293

Haven't tested this, but the general approach looks good to me.

src/plugins/expressions/common/expression_functions/specs/theme.ts

flash1293 · 2020-07-30T07:58:12Z

src/plugins/expressions/common/expression_functions/specs/theme.ts

+      help: i18n.translate('expressions.functions.font.args.defaultHelpText', {
+        defaultMessage: 'default value in case theming info is not available.',
+      }),
+      types: ['string', 'number'],


Can we just allow everything here? For palettes it might be nice at some point to return more than just a simple value at once - this would give us more flexibility in using theme variables.

An example would be the palette params - the shape can be different for different palettes, so we can't always flatten them out into separate args

Same for output type of course.

@ppisljar you gave a plus 1 but didn't change the code - do you have concerns with this? I'm open for discussion, but I'm not seeing why we shouldn't allow all types here

i did, for some reason clicking the link in above comment takes you to old code without this change. going to files to view all changes you can confirm that type limitation was removed.

Missed that, sorry. It seems like the Output typescript type is still more restrictive, but that's just a nit as it should only be relevant for the implementation of the function itself.

lukeelmers

Code here LGTM so far. Haven't tested locally yet, but would be nice if we could add a couple of basic unit tests for the theme fn.

src/plugins/expressions/common/expression_functions/specs/theme.ts

…e.ts Co-authored-by: Luke Elmers <lukeelmers@gmail.com>

ppisljar · 2020-08-03T12:06:53Z

@elasticmachine merge upstream

into expressions/theming

lukeelmers

Added 2 small notes, but overall code updates LGTM; tested out the font function in Canvas locally and it seems to still be working as I'd expect.

lukeelmers · 2020-08-05T19:15:52Z

src/plugins/expressions/common/expression_functions/specs/theme.ts

+export type ExpressionFunctionTheme = ExpressionFunctionDefinition<
+  'theme',
+  null,
+  Arguments,
+  Output
+>;


This should also get added to the mapping of all function definitions provided by the expressions plugin in src/plugins/expressions/common/expression_functions/types.ts

(I'm wondering if we should just get rid of this mapping; it feels like something that will be easy to forget to update?)

lukeelmers · 2020-08-05T19:19:28Z

src/plugins/expressions/common/expression_functions/specs/tests/font.test.ts

-      it('defaults to 14px', () => {
-        const result = fn(null);
-        expect(result).toMatchObject({
-          spec: {
-            fontSize: '14px',
-          },
-        });
-        expect(result.css).toContain('font-size:14px');
-      });


One nit here -- it would be cool to add an integration test for font that actually validates the correct default value is applied via theme, based on whether or not you have set the appropriate variable.

that won't be possible due to the way interpreter works:

before running any function (font in this case) it will check if any arguments are missing and if defaults are provided for them

it will evaluate the default value (interpret and execute), so if default is expression it will run it

it will provide the result of expression as an actual argument to our function (font)

here we are just testing the font function without interpreter and providing arguments in. to do integration test we would need interpreter service running (with the registries and everything) which i think is hard at the moment.

flash1293

Ready to merge from my side, just a small nit about the Output type of the function.

ppisljar · 2020-08-11T13:39:48Z

@elasticmachine merge upstream

* master: (106 commits) [Functional Tests] Adds a wait time between setting the index pattern and the time field on TSVB (elastic#74736) [Lens] Add styling options for x and y axes on the settings popover (elastic#71829) [Maps] add initial location option that fits to data bounds (elastic#74583) theme function (elastic#73451) [data.ui.query] Write filters to query log from default editor. (elastic#74474) [data.search.SearchSource] Move some SearchSource dependencies to the server. (elastic#74607) [Canvas][tech-debt] Convert renderers (elastic#74134) [security solutions][lists] Adds end to end tests (elastic#74473) pluralized for occurrences vs occurrence (elastic#74564) Update links that pointed to CONTRIBUTING.md (elastic#74757) [Ingest pipelines] Implement tabs in processor flyout (elastic#74469) [Event log] Use Alerts client & Actions client when fetching these types of SOs (elastic#73257) Bump chalk to 4.1.0 (elastic#73397) Index pattern field list - transition away from extending array - introduce and use getAll() (elastic#74718) [SECURITY] Bugs css/inspect (elastic#74711) [telemetry] update README to downplay ui_metrics (elastic#74635) Fixed grammar (elastic#74725) [Telemetry][API Integration] size_in_bytes to be a number (elastic#74664) [ILM] Convert node details flyout to TS (elastic#73707) [Ingest Node Pipelines] Sentence-case processor names (elastic#74645) ...

kibanamachine · 2020-08-11T17:55:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3aef92a

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
expressions	109	+1	108

page load bundle size

id	value	diff	baseline
expressions	348.7KB	+1.2KB	347.6KB

History

💚 Build #67497 succeeded 3aef92a
💚 Build #66943 succeeded 2265a21
💔 Build #66892 failed a384284
💔 Build #66060 failed 73979df
💚 Build #65524 succeeded e9bb13a

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…nes/processor-forms-a-d * 'master' of github.com:elastic/kibana: (25 commits) [ML] Removing full lodash library imports (elastic#74742) [Search] Server strategy example (elastic#71679) [Reporting] Fix and test for Listing of Reports (elastic#74453) [maps] fix drawing shapes (elastic#74689) [Resolver] Improve simulator. Add more click-through tests and panel tests. (elastic#74601) Deprecate schema-less specs in Vega (elastic#73805) [Security Solution] Rename Administration > Hosts subtab to Endpoints (elastic#74287) Timelion deprecation doc (elastic#74508) [Functional Tests] Adds a wait time between setting the index pattern and the time field on TSVB (elastic#74736) [Lens] Add styling options for x and y axes on the settings popover (elastic#71829) [Maps] add initial location option that fits to data bounds (elastic#74583) theme function (elastic#73451) [data.ui.query] Write filters to query log from default editor. (elastic#74474) [data.search.SearchSource] Move some SearchSource dependencies to the server. (elastic#74607) [Canvas][tech-debt] Convert renderers (elastic#74134) [security solutions][lists] Adds end to end tests (elastic#74473) pluralized for occurrences vs occurrence (elastic#74564) Update links that pointed to CONTRIBUTING.md (elastic#74757) [Ingest pipelines] Implement tabs in processor flyout (elastic#74469) [Event log] Use Alerts client & Actions client when fetching these types of SOs (elastic#73257) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/field_components/text_editor.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/manage_processor_form.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/append.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/bytes.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/circle.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/common_fields/field_name_field.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/common_fields/ignore_missing_field.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/convert.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/csv.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/date.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/date_index_name.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/dissect.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/dot_expander.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/drop.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/index.ts # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/shared.ts

stacey-gammon · 2020-09-09T12:44:05Z

Looks like this PR is missing the 7.{num} label, but it was backported. Can you add it @ppisljar for the release it went/is going into?

theme function

4a38577

ppisljar added WIP Work in progress Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:AppArch labels Jul 28, 2020

ppisljar requested a review from a team as a code owner July 28, 2020 13:59

ppisljar requested review from lukeelmers, flash1293 and clintandrewhall July 28, 2020 13:59

ppisljar added release_note:skip Skip the PR/issue when compiling release notes v8.0.0 labels Jul 28, 2020

fixing i18n

64c4dcc

flash1293 reviewed Jul 30, 2020

View reviewed changes

ppisljar added 2 commits July 30, 2020 06:54

fixing tests

d90c347

Merge branch 'master' into expressions/theming

e9bb13a

ppisljar removed the WIP Work in progress label Jul 30, 2020

lukeelmers reviewed Jul 30, 2020

View reviewed changes

src/plugins/expressions/common/expression_functions/specs/theme.ts Outdated Show resolved Hide resolved

Update src/plugins/expressions/common/expression_functions/specs/them…

18acd74

…e.ts Co-authored-by: Luke Elmers <lukeelmers@gmail.com>

elasticmachine and others added 3 commits August 3, 2020 06:06

Merge branch 'master' into expressions/theming

19b2ab0

adding test

2658d66

Merge branch 'expressions/theming' of https://github.com/ppisljar/kibana

73979df

into expressions/theming

lukeelmers approved these changes Aug 5, 2020

View reviewed changes

ppisljar added 2 commits August 6, 2020 06:15

updating

9acb737

Merge branch 'master' into expressions/theming

a384284

flash1293 approved these changes Aug 6, 2020

View reviewed changes

ppisljar added 2 commits August 6, 2020 08:17

fixing test

6fd6675

updating type

2265a21

Merge branch 'master' into expressions/theming

3aef92a

ppisljar merged commit 0ef17f9 into elastic:master Aug 11, 2020

ppisljar mentioned this pull request Aug 12, 2020

[7.x] theme function (#73451) #74825

Merged

ppisljar added a commit to ppisljar/kibana that referenced this pull request Aug 12, 2020

theme function (elastic#73451)

a136e63

ppisljar added a commit that referenced this pull request Aug 12, 2020

theme function (#73451) (#74825)

1b222c7

ppisljar mentioned this pull request Sep 9, 2020

theming support #71064

Closed

ppisljar added the v7.10.0 label Sep 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

theme function #73451

theme function #73451

ppisljar commented Jul 28, 2020 •

edited

Loading

elasticmachine commented Jul 28, 2020

flash1293 left a comment

flash1293 Jul 30, 2020

flash1293 Aug 6, 2020

ppisljar Aug 6, 2020

flash1293 Aug 6, 2020

lukeelmers left a comment

ppisljar commented Aug 3, 2020

lukeelmers left a comment

lukeelmers Aug 5, 2020

lukeelmers Aug 5, 2020

ppisljar Aug 6, 2020

flash1293 left a comment

ppisljar commented Aug 11, 2020

kibanamachine commented Aug 11, 2020

stacey-gammon commented Sep 9, 2020

theme function #73451

theme function #73451

Conversation

ppisljar commented Jul 28, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Jul 28, 2020

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Jul 30, 2020

Choose a reason for hiding this comment

flash1293 Aug 6, 2020

Choose a reason for hiding this comment

ppisljar Aug 6, 2020

Choose a reason for hiding this comment

flash1293 Aug 6, 2020

Choose a reason for hiding this comment

lukeelmers left a comment

Choose a reason for hiding this comment

ppisljar commented Aug 3, 2020

lukeelmers left a comment

Choose a reason for hiding this comment

lukeelmers Aug 5, 2020

Choose a reason for hiding this comment

lukeelmers Aug 5, 2020

Choose a reason for hiding this comment

ppisljar Aug 6, 2020

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

ppisljar commented Aug 11, 2020

kibanamachine commented Aug 11, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

page load bundle size

History

stacey-gammon commented Sep 9, 2020

ppisljar commented Jul 28, 2020 •

edited

Loading