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

Theme-aware tokens for graphs #783

Open
1 of 2 tasks
jattasNI opened this issue Oct 17, 2022 · 13 comments
Open
1 of 2 tasks

Theme-aware tokens for graphs #783

jattasNI opened this issue Oct 17, 2022 · 13 comments
Labels
enhancement New feature or request

Comments

@jattasNI
Copy link
Contributor

jattasNI commented Oct 17, 2022

😯 Problem to Solve

The PDV app in SLE is going to add a Plotly graph to their app. It would be good for Nimble to help them style it with theme-aware colors/fonts/spacing/line thicknesses.

image

💁 Proposed Solution

Ideas:

  1. Provide dedicated theme-aware tokens for the graph
  2. Update our documentation for existing generic theme-aware tokens to describe how they apply to graphs
  3. Ship a style sheet that applies tokens to a plotly graph
  4. Provide an example of applying tokens to a plotly graph
  5. A dedicated nimble-graph component that wraps plotly in a FAST template and applies the tokens automatically
    • in past discussions we've leaned away from this approach because the ROI seems low...it would take a lot of work to expose all the APIs for the graph in a custom element and apps tend to want to do a lot of customization of graphs

📋 Tasks

  1. client request
    m-akinc
@jattasNI jattasNI added enhancement New feature or request triage New issue that needs to be reviewed labels Oct 17, 2022
@m-akinc m-akinc removed the triage New issue that needs to be reviewed label Oct 18, 2022
@rajsite
Copy link
Member

rajsite commented Oct 18, 2022

I'm wondering if Plotly has the right hooks to be-aware of CSS Custom Properties to use for styling.

I expect that we actually need programmatic hooks into the theme changing and for querying the theme-aware token values programmatically from JS. FAST gives those APIs but we should find the right way to expose that to apps.

@jattasNI
Copy link
Contributor Author

jattasNI commented Oct 21, 2022

If this issue is accurate then Plotly still doesn't support styling via CSS plotly/plotly.js#1195 (comment)

But exposing tokens through our normal mechanism and adding APIs to convert them to JS values and to subscribe to theme changes seems like a good direction.

@rajsite
Copy link
Member

rajsite commented Oct 21, 2022

Some ideas for what the programmatic API could look like:

  • FAST gives a way with a DesignToken to get a value and to subscribe to value changes. We could expose FAST's DesignToken API directly (we don't expose the design token objects themselves yet through Angular). It's powerful, you can use it to write token values too which I'm not sure we should expose to users directly...

  • We could create a higher-level api that kinda looks like the browser Observer apis (ResizeObserver, IntersectionObserver):

    const themeObserver = new ThemeObserver(['application-background-color', 'font-color'], tokens => {
        console.log(tokens['application-background-color']);
    });
    // Calling observe will synchronously run the callback for the initial values
    // any further triggers should probably happen on a microtask instead of synchronously
    themeObserver.observe(myElement);
    themeObserver.disconnect();

    I kinda like the custom Observer approach because we can could choose what tokens we make accessible programmatically, also doesn't expose implementation details of our tokens. I bet we can also get some good typing for the token names and maybe even the token values (which, to be clear, the FAST Design Tokens already have, but I think this API could too).

@ni-ramya
Copy link

Is it possible to expose variables for fonts and font sizes as well? This will be required for styling the legends, graph titles, axis names, tec.

@jattasNI
Copy link
Contributor Author

Here is the first PR that wants to use these tokens. For now I'm suggesting using the tokens in design-tokens.ts to read the current value of certain tokens.

This also revealed a desire to export this API from nimble-angular (and presumably NimbleBlazor too someday) instead of just nimble-components.

When we fix this issue we should go and update that code to use the new API.

@jattasNI
Copy link
Contributor Author

Here's a PR pulling base token values (unaware of themes) from nimble-tokens.

@ni-ramya
Copy link

ni-ramya commented Mar 31, 2023

Hi @jattasNI @rajsite , we have a work item to make the reusable plotly component theme aware coming up. Any plans for bringing in the API anytime sooner? If yes we can incorporate it right away. If not we will go ahead with using the tokens in design-tokens.ts for now and later when the API is available we can replace them. Please let me know.

@jattasNI
Copy link
Contributor Author

@ni-ramya I believe our visual designer @RickA-NI is working on coming up with a definitive list of values for some of the missing tokens as we speak and hopes to have it ready next week. Once that's ready the Nimble dev work involved in the ideal solution is:

  1. expose those values in nimble-tokens. This would allow them to be used like this PR was doing but they won't change based on theme
  2. expose the theme-aware values in nimble-components in design-tokens.ts. This would allow the app to read the value of tokens for the current theme, but wouldn't offer a way to know when the theme has changed.
  3. expose an API to be notified when the theme has changed, as described in Milan's comment above

None of this is planned immediately by the Nimble team but I think it's mostly pretty easy to accomplish (though 3 requires a bit of API design). If your team is interested in contributing those changes we'd be happy to help you along. If not then we'd have to have a conversation about when Nimble team will be able to get to it.

@fredvisser
Copy link
Contributor

#787

@ni-ramya
Copy link

ni-ramya commented Apr 6, 2023

@jattasNI thanks for the reply. Since you have mentioned that the Nimble team does not plan it immediately, we would like to take it up. But we would also like to understand the efforts involved in it. Could you please briefly summarise the total effort involved in exposing the Nimble API? We can consider design discussions, development efforts, unit testing, documentation and any additional work that I may have missed. A ballpark number would be helpful in our commitment as well.

@noufal-ni

@jattasNI
Copy link
Contributor Author

jattasNI commented Apr 6, 2023

@ni-ramya that's great news!

Items 1 and 2 above should each only be a couple hours of work. If it's your first contribution then you'd probably want to pad that estimate with some time to get familiar with the repo and have some back and forth on your first PR. Maybe 3 days total?

Item 3 will require more discussion and testing so I'd suggest reserving about a week (plus any additional ramp up time if it's a different developer than the one who did 1 and 2).

@rajsite
Copy link
Member

rajsite commented Apr 19, 2023

Hey @ni-ramya, talked this over some more with Jesse and at the moment we aren't completely confident about implementing the Observer approach discussed above.

We need to think of the design a bit more, some things we need to figure out. I'll capture some of the discussions we had for our future reference:

  • Is this a use-case we expect to be common outside of nimble or is this a very-specific use case?
  • Will there be more UIs that need to be able to see the value of our CSS Custom Properties without being able to leverage stylesheets directly
  • Is the use-case common enough that we should encapsulate the implementation details of FAST, or is this essentially the workflow of building what is a nimble component outside of Nimble so it is okay to be aware of FAST (like our table columns)
  • Is this something Blazor applications should be able to integrate with easily (in which case we probably want an event API instead of a JS api)
  • Are there potentially future native apis we could align with, i.e. ComputedStyleObserver (need more research)

However, to unblock you today, I think you could accomplish leveraging the nimble CSS Custom Properties by using getComputedStyle, for example: window.getComputedStyle(viewChild).getProperty('--ni-nimble-background-color').

For an Angular component I think you would want to get the relevant CSS properties values on ngAfterViewInit. In addition, listen for changes to the SLThemeService and then recalculate the computed style for the nimble css custom properties related to graph styling. Hopefully observing those two scenarios are enough to stay in-sync with updates to the nimble realted css custom properties until we can think of an alternative way to observe for those changes.

vikash-ni added a commit that referenced this issue Jun 2, 2023
# Pull Request

## 🤨 Rationale
This PR has the update to add a design token for graph gridlines to make
it theme aware.

We have a graph visualization in the Data spaces module of the Test
Insights app. To make the graph theme aware, we have added a nimble
design token for the graph gridlines.

This is part of #783.

<!---
Provide some background and a description of your work.
What problem does this change solve?

Include links to issues, work items, or other discussions.
-->

## 👩‍💻 Implementation
- Added a design token called `graphGridlineColor`

<!---
Describe how the change addresses the problem. Consider factors such as
complexity, alternative solutions, performance impact, etc.

Consider listing files with important changes or comment on them
directly in the pull request.
-->

## 🧪 Testing
All existing tests should pass

<!---
Detail the testing done to ensure this submission meets requirements. 

Include automated/manual test additions or modifications, testing done
on a local build, private CI run results, and additional testing not
covered by automatic pull request validation.
-->

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.

---------

Co-authored-by: Vikash Venkatesh <[email protected]>
@jattasNI
Copy link
Contributor Author

jattasNI commented Jun 7, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Backlog
Development

No branches or pull requests

5 participants