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

theming support #71064

Closed
ppisljar opened this issue Jul 8, 2020 · 3 comments
Closed

theming support #71064

ppisljar opened this issue Jul 8, 2020 · 3 comments
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline)

Comments

@ppisljar
Copy link
Member

ppisljar commented Jul 8, 2020

in order to support theming across all solutions using expressions we should align on what a theme is, how its used and a set of functions to work with it.

Charts should have arguments allowing to set styling properties like font, palette, line options. They should all use the same context type for those argument (font for font, palette for color palette, ….). For each of those types we provide a set of functions to generate those types.

Lets take a look at example pie function which has two arguments font and palette:

 {
    name: 'pie',
    aliases: [],
    type: 'render',
    inputTypes: ['pointseries'],
    help,
    args: {
      font: {
        types: ['style'],
        help: argHelp.font,
        default: '{font}',
      }, 
     palette: {
        types: ['palette'],
        help: argHelp.palette,
        default: '{palette}',
      },
  }
}

font function can generate font context type. It has a bunch of arguments, which all have defaults

  args: {
    align: {
      default: 'left',
      help: i18n.translate('expressions.functions.font.args.alignHelpText', {
        defaultMessage: 'The horizontal text alignment.',
      }),
      options: Object.values(TextAlignment),
      types: ['string'],
    },
    color: {
      help: i18n.translate('expressions.functions.font.args.colorHelpText', {
        defaultMessage: 'The text color.',
      }),
      types: ['string'],
    },
    family: {
      default: `"${openSans.value}"`,
      help: i18n.translate('expressions.functions.font.args.familyHelpText', {
        defaultMessage: 'An acceptable {css} web font string',
        values: {
          css: 'CSS',
        },
      }),
      types: ['string'],
    },

Proposal: We change those defaults to {theme 'font.family'} where theme function retrieves the key provided from a theme service. A property to this function can also be a name to select which theme you want to use (in case we would want to override selected theme on per-expression basis) and default to provide default value in case theme information is not available.

If expression function (chart) wants a different default than just {font} (wants to define the default font to something else than what font function defines) there are two options:
- Chart don't want the theme information to override this:
Set default to {font family='arial'}
- Chart would still want the theme info to override this if its present
Set the default to {font family={theme 'font.family' default='arial'}}

Proposed theme function arguments:

 {
    name: 'theme',
    aliases: [],
    args: {
      variable: {
       types: ['string'],
        aliases: ['_', 'var'],
        required: true
      }, 
     default: {
        types: ['string', 'number'],
      },
    name: {
      types: ['string'],
    }
  }
}

Benefits:

  1. Chart functions don't need to know anything about themes, they just know about their styling properties. Only functions to generate the styling context types know about themes and only theme function has access to theming service.

If we were to implement different theming service in canvas vs dashboard, canvas would only need to override theme function, everything else would stay the same (not that we should actually do that, we should go with one theming implementation imo)

  1. Allows us to implement themes for any visual property (actually for every argument), allows us to introduce global overrides at a later point as well as user overrides.

  2. No need to migrate anything. Canvas functions can stay exactly the same. Canvas stored expressions can stay exactly the same, lens expressions can stay exactly the same. (we'll take a look at this under examples)

  3. Support for embeddables comes almost for free (we'll take a look at this under variables)

Examples:

Lets take a look at some expressions, and how would they look after interpreter parses them and inserts default arguments.

  1. A simple pie expression with all the arguments defaulted

    • pie
    • pie font={font} palette={palette} interpreter inserted default arguments
    • pie font={font family={theme 'font.family'} size={theme 'font.size'}} palette={palette name={theme 'palette.name'}}}
      interpreter inserted default arguments for font and palette functions as well

we can see that a stored expression with pie function where styling arguments were defaulted (not set) will suddenly start supporting the themes

  1. A pie expression with some arguments set (not-defaulted)

    • pie font={font size=26}
    • pie font={font size=26 family={theme 'font.family'}} palette={palette name={theme 'palette.name'}}
      interpreter sets the defaults for the missing arguments (defaulted), for which theming will work,
      for arguments that were overriden the theming won't override them (expected behaviour)
  2. Setting a different default on a chart function then the default of styling function is

    • pie font={font family={theme 'font.family' default='arial'}} if theme 'font.family' is not set use arial default

Implemented with variables:

pie font={font family={theme default='arial' } size={theme 'font.size'}}
pie font={font family={var 'theme.font.family' default='arial' } size={var 'theme.font.size'}

Everything needed to implement this is already in place, steps we need to take to make this a reality:
- agree on theming variables we'll use
- change default values of styling functions to use the agreed on variables:
- For example: font function default for size is 14. we need to change it to
var 'theme.font.size' default=14 which will give us same result if theme info is not present,
-> but if present it will use theme information
- Give variables to executor (and build UI around them)
- Add variables to embeddable base input (to make it work with embeddables directly (inside and outside of canvas))

Concerns:
- We loose type safety (as we anyway do with variables. Variable can be of any type so var function can return anything and we can't check that at compile time but only at runtime)
- Could we have default/forced theme variable for specific arguments ? To avoid using wrong variables for wrong arguments
- We should have theme variables as a separate high level concept to prevent colisions

Proposal 2:

Similar to proposal 1, but to solve type safety concert and concert with having wrong variables specified: instead of having a special theme function each of the styling functions should work directly with the theming service. Styling functions should have no default arguments defined on the expression function definition, but rather have the defaults asigned inside the function when the argument is undefined.

Benefits:

  1. Chart functions don't need to change but styling functions do. However each styling function needs access to the theming service.

  2. Implementing of user overrides or global overrides might be more complicated as we'll need to do that in several styling functions (vs single function in proposal 1), but we might make that easier providing a set of utility functions each styling function would then use. (like getThemeProperty('font.family', 'arial') and that function internally can check the theme, global overrides, user overrides)

  3. We still don't need to migrate anything. No arguments to any functions would be changed, just styling function implementations would know that when there is no argument provided they should check with theming service for availability of it.

  4. Support for embeddables still comes almost for free if we would be using variables for passing down theme information. Another option would be a global theming service (which imo should be avoided).

  5. We avoid 1st and 2nd concert of proposal 1 with the downside of multiple functions needing access to theming service and implementations of those styling functions will be more complicated.

Examples:
pie
pie font={font} // interpreter inserts default argument, however font doesn't have defaults set for any of its arguments
// Internally font uses a getThemeProperty function when arguments were not given and provide default for when theme is not available

TLDR:

No matter which of above proposals we would go with we first need to agree on context types for our visual attributes (font, palette, ....), provide base set of functions to work with those context types and have all charting functions exposing arguments allowing to set this. Once that is in place we should be able to implement theming without breaking backward compatibility or needing migrations.

@ppisljar ppisljar added Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:AppArch labels Jul 8, 2020
@elasticmachine
Copy link
Contributor

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

@flash1293
Copy link
Contributor

@ppisljar It looks like we already implemented the proposal here, can we close this issue?

@ppisljar
Copy link
Member Author

ppisljar commented Sep 9, 2020

closed with #73451

@ppisljar ppisljar closed this as completed Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline)
Projects
None yet
Development

No branches or pull requests

3 participants