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

Textures fills in place of colors #1061

Closed
3 tasks done
nickofthyme opened this issue Mar 8, 2021 · 13 comments · Fixed by #1138
Closed
3 tasks done

Textures fills in place of colors #1061

nickofthyme opened this issue Mar 8, 2021 · 13 comments · Fixed by #1138
Assignees
Labels
:accessibility Accessibility related issue enhancement New feature or request kibana cross issue Has a Kibana issue counterpart released Issue released publicly :styling Styling related issue

Comments

@nickofthyme
Copy link
Collaborator

nickofthyme commented Mar 8, 2021

Is your feature request related to a problem? Please describe.
From kibana issue...

We have employees who are color blind, and can't consume data in most visualizations we build. This request is to solve the issue for these folks, which I assume could be accomplished through allowing for texture fills to be applied to different data series.

Describe the solution you'd like
The ability to add texture fills to data series. Using the CanvasRenderingContext2D.createPattern api.

Describe alternatives you've considered
Different color palettes.

Kibana Cross Issues
elastic/kibana#10726

Checklist

  • this request is checked against already exist requests
  • every related Kibana issue is listed under Kibana Cross Issues list
  • kibana cross issue tag is associated to the issue if any kibana cross issue is present
@nickofthyme nickofthyme added enhancement New feature or request kibana cross issue Has a Kibana issue counterpart :styling Styling related issue :accessibility Accessibility related issue labels Mar 8, 2021
@ron-debajyoti
Copy link
Contributor

Hi @nickofthyme!
As a beginner-level contributor to this project, is it advisable to take up this issue? Since this is a kibana cross issue with no beginner tag, I thought it would be better to ask before working on it.

@nickofthyme
Copy link
Collaborator Author

Hey @ron-debajyoti yeah totally!

I went through the issues the other day to see which are good first issue but couldn’t find many. I think many of our issues are fairly complicated to beginners except for maybe some of the bugs.

That said I think we can help you make the changes in the right places for this issue if you want to take it on.

I'm not sure exactly how the API would look for this but I imagine it would look similar to how we assign colors to each series. If you want to take a stab at it and put up a draft PR as a POC we can take a look and go from there. Or you could come up with an api for us to review and post it on this issue before jumping into code.

How does that sound?

@ron-debajyoti
Copy link
Contributor

@nickofthyme Thanks for the advice! I want to take this up and first try to get familiar with existing code and then think of a possible solution.
Maybe an API for this would be good.

@markov00
Copy link
Member

As @nickofthyme described, the canvas.createPattern method is the way to go with our current rendering implementation.
I think the example on that document page https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/createPattern#creating_a_pattern_from_a_canvas is also a valid one for an ideal solution:

  • creating an offscreen canvas element (by offscreen I mean: don't add it to the DOM, not using the OffscreenCanvas that is currently unsupported on some browser)
  • provide an SVG Path to render the pattern
  • use that canvas element as image for the createPattern method.

The complete task can be quite complex, but I think it can be splitted into multiple tasks:

  • research: quickly test on area or bar rendering how the createPattern works by experimenting in the renderPathFill method in src/chart_types/xy_chart/renderer/canvas/primitives/path.ts this method is used to apply a fill color to an area path
  • refactor: the renderPathFill and renderAreaPath should be able to use both a Fill or a Pattern
  • refactor: the src/chart_types/xy_chart/renderer/canvas/areas.ts renderArea method is the main one render an area, we should be able to change the way the used method buildAreaStyles works in order to accept both a static color or an SVG path to be used as pattern. In this case probably the AreaStyle should be extended to accept also an SVG Path and few other properties like size of the pattern
  • refactor: apply this also to bar rendering

We could also provide a set of predefined textures/pattern to apply, a good example is https://riccardoscalco.it/textures

@ron-debajyoti
Copy link
Contributor

Hi @nickofthyme @markov00, I'm very sorry for not following up on this. I was going through the codebase and took up this issue primarily because EUI was participating in GSOC and the proposed projects were very interesting and appealing to me. I was really into it, but since Elastic didn't get into the list of participating organizations, unfortunately, I had to set aside this issue and look for other organizations. I'm sorry for not communicating this earlier before.

Now that the student application period for GSOC is almost done, I would like to pick up from where I left things. Since this issue is a bit complex, probably its best that I make a draft PR first with the procedure @markov00 suggested.

@nickofthyme
Copy link
Collaborator Author

Hey @ron-debajyoti I totally understand, I appreciate you following up now. Also please don't feel obligated to complete this issue just because you started discussing it thinking it would be part of GSOC. If you would like to contribute we would be very grateful and happy to help you complete this issue, but I don't want you to feel pressured into completing this issue just because you started the discussion.

That said, if you do choose to contribute, yes I would take @markov00's suggestion creating a draft pr with the tasks he listed above. If you have any doubts he and I could clear things up.

@ron-debajyoti
Copy link
Contributor

ron-debajyoti commented Apr 16, 2021

Hey @nickofthyme I want to take this on just out of pure interest and appreciation for Elastic, nothing more 😃 .

I followed the first two steps @markov00 mentioned and have a few things to discuss:

  1. I tried using an SVG to render the pattern, but the since we have to wait for the browser to load the svg, it's causing an improper pattern fill , due to ctx being changed asynchronously somewhere else.
    A demo image for reference would be:
    story1
    I'm still yet to figure out this bug.

  2. I tried another way of just creating checkboxes and filling them with appropriate fill colors imported, but since they aren't a pattern technically, probably this won't work for our case. The graphs came out as follows:
    sotry2

Would appreciate your feedback to proceed accordingly.

@nickofthyme
Copy link
Collaborator Author

Hey @ron-debajyoti, that's great to hear! 🎉

That is looking great, I'm not too familiar with the canvas pattern API but I am happy to take a look at what you have so far and see what's happening. Would you mind opening a draft PR with what you have so far, doesn't need to be pretty or conform to the contributing guidelines at this point.

@markov00
Copy link
Member

I want to take this on just out of pure interest and appreciation for Elastic, nothing more 😃 .
We really appreciate it ❤️

I think your second approach looks great and follows what we have in mind: using canvas pattern instead of SVG, and creating a simple pattern to be used as fill.
I've created a simple example, outside charts, of how I think this could implemented: https://codesandbox.io/s/thirsty-jones-w3k49?file=/src/index.js
there are definitely a lot of improvements like:

  • limiting the number of created offscreen canvas
  • extending the number of patterns
  • avoiding double rendering the path and using directly the fillStyle within the pattern itself

@ron-debajyoti
Copy link
Contributor

I think your second approach looks great and follows what we have in mind: using canvas pattern instead of SVG, and creating a simple pattern to be used as fill.

Yeah, right now I'm using this method and textures.js as we need to make different textures for different areas in graphs like stacked areas, band areas, etc.

there are definitely a lot of improvements like:

  • limiting the number of created offscreen canvas
  • extending the number of patterns
  • avoiding double rendering the path and using directly the fillStyle within the pattern itself

I'm trying to accommodate as much improvements possible and make the draft PR.

@ron-debajyoti
Copy link
Contributor

Hi @nickofthyme @markov00 !
I made the draft PR where I made a basic implementation and some points for further work to be done (Todo). Perhaps we could use that to plan our textures API, the types of patterns we are gonna support., etc.

@ron-debajyoti
Copy link
Contributor

Hi!
Any update on this issue or any feedback on how do we proceed?

@nickofthyme
Copy link
Collaborator Author

🎉 This issue has been resolved 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
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 kibana cross issue Has a Kibana issue counterpart released Issue released publicly :styling Styling related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants