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

Make TracePage components themeable [Subtask] #509

Closed
aocenas opened this issue Jan 13, 2020 · 5 comments
Closed

Make TracePage components themeable [Subtask] #509

aocenas opened this issue Jan 13, 2020 · 5 comments

Comments

@aocenas
Copy link

aocenas commented Jan 13, 2020

Requirement - what kind of business use case are you trying to solve?

Subtask from #507

Problem - what in Jaeger blocks you from solving the requirement?

Proposal - what do you suggest to solve the problem or improve the existing situation?

The resulting package should provide some theme-ability options. For the Grafana usecase there is concern of making the look and feel more aligned with other parts of Grafana and also that we provide dark/light theme.

There are 2 main ways to do this:

  • css3/scss/less styles with overridable variables. Right now Jaeger UI uses plain css that is loaded by webpack so may be closer to what is used right now. It is also used in AntD that Jaeger UI already uses https://ant.design/docs/react/customize-theme. On the other hand, consumers need to wire up the styling solution in their webpack (or other build system) configuration.
  • CSS in JS solutions. This allows for theming purely in JS/TS context by using ThemeProvider wrappers. This approach is used for example in MaterialUI https://material-ui.com/customization/theming/. This allows for less configuration and theming can be to some extent type safe if used with Typescript. Also Grafana uses CSS in JS solution heavily (EmotionJS).

Apart from changes in styles there may be a need for injection of custom components. For example using a custom button or icon component may be more reasonable than trying to manipulate just the styles through theming. This could be done either by having optional props or by provider pattern.

Any open questions to address

@everett980
Copy link
Collaborator

@aocenas I considered commenting on PR#507 but I think it makes sense to consolidate this discussion here.

Also Grafana uses CSS in JS solution heavily (EmotionJS)

What led you/others at Grafana to land on EmotionJS?
I haven't yet fully gone down the rabbit hole of css-in-js, but it does seem like Emotion is running away from the competition since ~September (npmtrends)

@dprokop
Copy link

dprokop commented Feb 21, 2020

@everett980 please take a look at our overview here: grafana/grafana#15438

The most important factor for us was to be able to effectively theme components based on component props/state as well as have styles as close to the component as possible. Important thing with Emotion was also the fact that we could switch the styled vs className paradigms (descibed in the issue above) easily without the need of changing the framework. Currently we use emotion package (className approach) but soon we will migrate to @emotion/core and css property.

@everett980
Copy link
Collaborator

Thanks @dprokop

Emotion css prop
This is a very neat feature of Emotion. Instead of using classNames approach you apply styles to a component via css prop. Emotion does it's magic and generates class names for you.

Unfortunately, it's tricky to integrate with TS and introduces some complexity to our architecture:

Did you figure out a way to use the css prop with TS? Getting type checking for css-in-js would be nice. I read the issue on emotion, and it seems feasible to have the css prop without losing React.Fragment short hand. Though it may be tricky to add to JaegerUI as we use react-app-rewired.

@aocenas
Copy link
Author

aocenas commented Feb 26, 2020

@dprokop what is the status of migrating to v10 of emotion on our side?

@everett980 Here I used the older framework agnostic way to use emotions without the css prop exactly because I wasn't sure how it will affect the react app setup and thought it would be better leave it out for later if needed.

@jkowall
Copy link
Contributor

jkowall commented Oct 28, 2021

Grafana decided to do their own fork to reskin, so I am closing this out.

@jkowall jkowall closed this as completed Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants