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

feat(a11y): add textures to fill options #1138

Merged
merged 27 commits into from
Jun 8, 2021

Conversation

ron-debajyoti
Copy link
Contributor

@ron-debajyoti ron-debajyoti commented Apr 24, 2021

Summary

Adds optional texture styles prop to RectStyle and AreaStyle for xy area and bar charts.

Screen.Recording.2021-05-27.at.04.25.06.PM.mp4

Details

Adds new TextureStyle interface below.

interface TexturedStylesBase {
  /** polygon fill color for texture */
  fill?: Color | ColorVariant;
  /** polygon stroke color for texture */
  stroke?: Color | ColorVariant;
  /** polygon stroke width for texture  */
  strokeWidth?: number;
  /** polygon opacity for texture  */
  opacity?: number;
  /** polygon opacity for texture  */
  dash?: number[];
  /** polygon opacity for texture  */
  size?: number;
  /**
   * The angle of rotation for entire texture
   * in degrees
   */
  rotation?: number;
  /**
   * The angle of rotation for polygons
   * in degrees
   */
  shapeRotation?: number;
  /** texture spacing between polygons */
  spacing?: Partial<Point> | number;
  /** overall origin offset of pattern */
  offset?: Partial<Point> & {
    /** apply offset along global coordinate axes */
    global?: boolean;
  };
}

interface TexturedShapeStyles extends TexturedStylesBase {
  /** typed of texture designs currently supported */
  shape: TextureShape;
}

interface TexturedPathStyles extends TexturedStylesBase {
  /** path for polygon texture */
  path: string | Path2D;
}

/**
 * @public
 *
 * Texture style config for area spec
 */
export type TexturedStyles = TexturedPathStyles | TexturedShapeStyles;

Limitations

Unable to be assigned across splitSeriesAccesors like color, thus limited to separate Series definitions. Could be improved in the future to make ___SeriesStyle applied per XYSeriesIdentifier.

Connected issues

fix #1061 - Adds textures styles for xy area and bar charts

Todos:

  • For this PR, I added the majority of code in xy_chart/renderer/canvas/primitives/path.ts. We need to modularize it so that it can be added to different charts.
  • We need to standardize the number of textures to support so that different areas in a single chart can support different textures.

Checklist

  • Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
  • This was checked for cross-browser compatibility
  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • Unit tests were updated or added to match the most common scenarios

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@ron-debajyoti ron-debajyoti changed the title feat (a11y) : A draft PR for adding textures with colors. #1061 feat (a11y) : A draft PR for adding textures with colors. Apr 24, 2021
@ron-debajyoti
Copy link
Contributor Author

Adding an image of the result :
image

@ron-debajyoti
Copy link
Contributor Author

Hi @nickofthyme @markov00,
Any update on how do we proceed further? I was thinking first designing the API, considering no. of textures we support and possibly adding it in utils and then making a list of charts we initially support.

@nickofthyme
Copy link
Collaborator

@ron-debajyoti sorry for the delayed review. I'll take a look at this today. @markov00 is out sick today but should be back tomorrow to review as well.

@nickofthyme nickofthyme changed the title feat (a11y) : A draft PR for adding textures with colors. feat(a11y): A draft PR for adding textures with colors. May 3, 2021
@nickofthyme nickofthyme changed the title feat(a11y): A draft PR for adding textures with colors. feat(a11y): add textures to fill options May 3, 2021
Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

This is fantastic! Thanks, again for your work on this. Also much less complex than I expected it to be.

I left a few comments that came to mind I think we could expose some options on the Theme that would allow the user to better control the pattern as well as provide a small subset of simple patterns to choose from as you mentioned in the TODO's.

@ron-debajyoti
Copy link
Contributor Author

Hi @nickofthyme!
Sorry for replying late. I got busy with my coursework earlier last week and got some time right now to continue working on this.

I started implementing the API, based on the feed you gave earlier and so far I have considered this interface for TexturedStyles

export interface TexturedStyles {
  /* typed of texture designs currently supported */
  type: 'line' | 'circle' | 'square' | 'triangle' | 'cross';

  /* To fill the stoke with Color or ColorVariant */
  stroke: Color | ColorVariant;

  /* The angle of rotation required in radians */
  rotation: number;

  /* The opacity of each texture on the theme/series */
  opacity?: number;

  /* If the polygon textures should be filled or empty */
  fill?: 0 | 1;

  /* The size of the textures,defaulted as 10 */
  scale?: number;

  path?: string;
  spacing?: number;
}

and updated the AreaStyle to incorporate TexturedStyles as well. I incorporated the rotation and opacity and other attributes but am still in the process of incorporating TexturedStyles.path and TexturedStyles.spacing.

I'm a bit confused about how are we going to start filling the textures automatically. For a start, I was thinking of looking into this line for xy_charts, but I'm confused about how do we automate the selection of texture. Is there a way to maybe add the texture data to every areaChart ?

Meanwhile, should I open a new PR with the latest updated code, since the current PR is just a draft one for discussion ?

@markov00
Copy link
Member

Meanwhile, should I open a new PR with the latest updated code, since the current PR is just a draft one for discussion ?

Hi @ron-debajyoti thanks for all your help here! If you like to open a new PR with an updated code is fine.
What do you think if we jump directly on the PR helping you on the integration part with the Theme style?
I think the most of core part of the feature is done by you and we can just add the glue to the missing pieces.

@ron-debajyoti
Copy link
Contributor Author

What do you think if we jump directly on the PR helping you on the integration part with the Theme style?
I think the most of core part of the feature is done by you and we can just add the glue to the missing pieces.

Yeah, that would be great as well.
I pushed the latest update yesterday to this PR itself, need to refactor it a bit. Perhaps you could review it and guide me the integration part.

@ron-debajyoti
Copy link
Contributor Author

Hey @nickofthyme @markov00,
A gentle reminder that it would be helpful if the latest changes are reviewed and provide some guidance towards the integration part.

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

hi @ron-debajyoti I've wrote some comments on the PR that should help you complete the work.
In particular how to get the texture from the style object of the geometry and the request to apply the pattern also to bars
Thanks a lot for that huge work

src/utils/themes/theme.ts Show resolved Hide resolved
src/utils/themes/textures.ts Outdated Show resolved Hide resolved
src/utils/themes/theme.ts Outdated Show resolved Hide resolved
src/utils/themes/theme.ts Outdated Show resolved Hide resolved
src/utils/themes/theme.ts Outdated Show resolved Hide resolved
ron-debajyoti and others added 3 commits May 20, 2021 19:14
- use shared svg shape paths
- pull texture styles from theme
- add more options to texture theme styles
- add global pattern rotation and offset
- add stories to showcase changes
- dry up degree to radian conversions
- Add texture option to rect styles
- improves shared geom highlight opacity usage
- set default rectBorder.strokeWidth to 1, visible will hide
- improve stories with bar series option
@nickofthyme nickofthyme added :accessibility Accessibility related issue :styling Styling related issue enhancement New feature or request labels May 27, 2021
@nickofthyme
Copy link
Collaborator

nickofthyme commented May 27, 2021

@ron-debajyoti I made a few changes to the great start you provided, I hope you don't mind me pushing to your branch 😅. I think this is for the most part complete in terms of the overall implementation, @markov00 let me know if I'm missing something.

I wired up the texture styles to the theme and passed a few global options to change the overall rotation and offset of the pattern. Also added the textures to the bar rect styles.

I also added two stories to play with the textures in our storybook. To see them you can just run...

yarn && yarn start

Then visit either and play with the knob inputs:

I think the only thing remaining is to fix the unit and visual regression tests. I can look at those tomorrow unless you'd like to take a stab at it.

Anyways, lmk if you have any questions or comments on the changes. I'd be happy to explain.

@nickofthyme nickofthyme marked this pull request as ready for review May 28, 2021 22:51
@ron-debajyoti
Copy link
Contributor Author

Hey @nickofthyme @markov00 ,
I believe most of the integration work is already done by @nickofthyme. Honestly, the ending work couldn't have been at the same level of finesse by me and I learned a lot through these contributions. Thanks a lot once again for allowing me to contribute. Perhaps we could merge and close this.

On another note, do you think is it a good idea for me to take up some of the other open beginner level issues of this project despite not being a core member of elastic? I got a good overall feel of the codebase and would love to improve my skills esp. writing modular code.

@nickofthyme
Copy link
Collaborator

Jenkins test this please

@nickofthyme
Copy link
Collaborator

Jenkins test this please

@nickofthyme
Copy link
Collaborator

@markov00 both stories seem to work fine with bars for me. Let me know if you see something different.

Screen.Recording.2021-06-07.at.11.43.26.AM.mp4

@nickofthyme
Copy link
Collaborator

nickofthyme commented Jun 7, 2021

On another note, do you think is it a good idea for me to take up some of the other open beginner level issues of this project despite not being a core member of elastic? I got a good overall feel of the codebase and would love to improve my skills esp. writing modular code.

@ron-debajyoti If you'd like to take on another issue I compiled a list of potentially good issues to tackle under the good first issue label. If you'd like to take a look at those and pick one you'd like to focus on we can refine the scope of the issue and make sure we have a clear understanding of the solution.

@nickofthyme
Copy link
Collaborator

Jenkins retest this please

@nickofthyme
Copy link
Collaborator

Jenkins test this please

@nickofthyme nickofthyme merged commit fd0479f into elastic:master Jun 8, 2021
nickofthyme pushed a commit that referenced this pull request Jun 8, 2021
# [30.1.0](v30.0.0...v30.1.0) (2021-06-08)

### Features

* **a11y:** add textures to fill options ([#1138](#1138)) ([fd0479f](fd0479f))
@nickofthyme
Copy link
Collaborator

🎉 This PR is included in version 30.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickofthyme nickofthyme added the released Issue released publicly label Jun 8, 2021
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:accessibility Accessibility related issue enhancement New feature or request released Issue released publicly :styling Styling related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Textures fills in place of colors
5 participants