Closed kuzhelov closed 5 years ago
@levithomason's response:
I'm not sure we should move forward with this pattern just yet. It is a big change that has many implications. I also spent some time discussing it with Fabric yesterday and I think we reached a few good conclusions:
It seems there are only two options here.
Using many variables does have advantages over using functions:
Using variable functions has a few downsides:
Here's how we could use the existing props
passed to our style function to override the icon style when used in the Radio:
const radioStyles = {
root: ({ props, variables }) => ({
'.ui-icon': {
color: props.checked
? variables.checkedIconBackgroundColor
: variables.uncheckedIconBackgroundColor
}
})
}
Although styles aren't as robust as updating the Icon's variables, it does save us from having to add JS listeners. In the variable function example, we'd need to pass in the hover
state of the Radio if we wanted to update the icon when the Radio was being hovered. That means we'd need mouse enter/leave listeners on the Radio, set state on change, then pass those values into the variable function. This will not only cause React renders but also re-evaluate the style function every time. It also means consumers and 3rd parties would need to follow that same pattern when using our variables functions.
Instead, if we use the "many variables" approach, we can just use CSS selectors for hover, active, focus states:
const radioStyles = {
root: ({ props, variables }) => ({
':hover': {
'.ui-icon': {
color: 'red'
}
}
})
}
There are obviously cons to this approach as well, but I'm thinking the main points are:
Thoughts?
Let me, please, directly challenge the examples of styling Radio
's icon you've suggested:
const radioStyles = {
root: ({ props, variables }) => ({
'.ui-icon': {
color: props.checked
? variables.checkedIconBackgroundColor
: variables.uncheckedIconBackgroundColor
}
})
}
Unfortunately, this approach is not robust enough - more than that, for some scenarios (including the one you've suggested) it simply won't work. Let's face it - here our intent is to define icon's color, and previously variables were able to abstract us away from the implementation details around how color is applied to icon. Now, in contrast, we have a problem: what if it will be an SVG icon? in that case it won't be styled properly, as SVG icons generally use fill
property (not color
) for coloring.
In general, in case of parent-child (component-slot) relationship variables serve like an interface that introduce robustness in expressing intents from parent to child component. Without this interface there are no ways for us to reliably introduce styles and guarantee that on the next day, with implementation approach being changed for child component, styles will remain to work properly.
Another question is about .ui-icon
- what does it mean? Is it about naming property of classes
object (why not use icon
as we've done before than), or it is about introducing static CSS styles (this will be applied globally to the page, what we probably won't like to have).
My opinion on that is the following: we should either
props+state
is passed as an argument to variables function. The reason for that is that this approach solves the problem of styling child components in a robust way - in contrast to any approach that will be about tweaking CSS of child component directlymany variables
approach mentioned above, we should consider to introduce prop+state
argument to slot's variables only. Although it will increase complexity, it is worth to notice that in general this pattern could be omitted by developer - and used only in cases where needed (and Radio
is a good example for that)
// radioVariables
export default (siteVariables) => ({
...
icon: ({ props }) => ({
...
color: props.checked
? checkedIconBackgroundColor
: uncheckedIconBackgroundColor
})
})
@levithomason's response:
Agree, however, compare the alternative. I'm afraid there isn't a reasonable alternative here. If there is, please elaborate as I'd like to not have to go this path.
The challenge of handling child style implementation details exists but is not a blocker by any means. It is already required to be solved in the theme whether or not we use child selectors.
The knowledge of the icon style implementation is already owned by the Teams theme in the Icon styles. The Icon style itself must handle all the correct cases of the icon and correctly apply variables in those cases (color vs fill for instance). It seems reasonable to me then that the Radio style (in the same theme) would also have this implementation knowledge. The implementation knowledge of the Icon's style is not leaking beyond the scope of the Teams theme package there.
The corresponding issue with variable functions is, how would a parent component change a slot variable on hover? I can see no other way than for the parent to have mouse enter/leave listeners on the parent to update its state and pass it to the variable function. I think this is a blocker opposed to a challenge. We cannot reasonably put our seal of approval on a pattern that requires every UI component on the page to have JS listeners for hover, active, and focus just so variable functions can have access to those states.
Let me, please, respond to each of the points raised:
Child Icon styles is a challenge
Actually, I see it being more like a blocker. Let me follow up on the example that you've suggested:
The Icon style itself must handle all the correct cases of the icon and correctly apply variables in those cases (color vs fill for instance). It seems reasonable to me then that the Radio style (in the same theme) would also have this implementation knowledge
Essentially, this applies something like the following:
// in radioStyles
// importing this 'knowledge' of how styles should be applied from Icon
import { applyIconStyles } from './Icon/iconStyles'
{
...
icon: ({ props, variables }) => applyIconStyles({ props, variables })
}
Although it seems to be a simple solution, its simplicity is very deceptive. First thing that we might consider is that now we would need to provide applyStyles
function that will be dependent on Radio's props
and variables
- this introduces an unreasonable coupling between these two, and it will quickly become to be brittle and nightmare to maintain.
But if we would proceed on coupling, there is much more serious problem here - now the functionality of applying Icon
styles is hardcoded for Radio
, so there won't be any possibility to put any other component to this slot. Variables are able to handle this problem much more gracefully, and with TS support this 'interface' between Radio
and any child slot component could be explicitly enforced.
Specifically, here is a way it would look like:
// radioVariables
// notice that there are no hard dependences declared here
// icon slot's component should only support 'color' variable and now how to handle it
{
...
icon: ({ props }) => {
color: props.checked ? checkedColor : color
}
}
Note that here the only thing that we are requiring from icon slot's component is to support color
variable - and nothing more. And even if we will introduce additional variables (to address needs of some Foo
component that will have borderColor
variable needed to be defined as well) - it will still be much more succinct and generic interface without any hardcoded dependencies!
Child Icon variable function is a blocker
Actually, don't see it being a blocker. The reason it is seen as blocker might come from this:
how would a parent component change a slot variable on hover? I can see no other way than for the parent to have mouse enter/leave listeners on the parent to update its state and pass it to the variable function
There is a crucially important thing to notice here - with rendered element's pseudo state being changed on 'hover', there is actually no state (in general sense, i.e. props+state) changed for the component .
With that, lets return back to our case of Radio
component. This is just one of many options how it could be handled in variables - probably, the simplest one:
// radioVariables
(siteVariables) => {
...
icon: ({ .. }) => {
..
colorOnHover: variables.colorOnHover
}
}
// iconStyles
{
root: ({ variables }) => {
...
':hover' : {
color: variables.colorOnHover
...
}
}
}
Thus there is no need to introduce any handling of hover
state changes in JS - and what is more important, everything that is possible to be achieved with styles is possible to achieve through much more generic (with no hardcoded dependencies) interface of variables.
Let's pair program on a prototype for this. I think it will help communication.
Thus there is no need to introduce any handling of hover state changes in JS
The issue isn't icon needing to apply color on hover of its own root. The issue is when icon needs to receive color on hover of the parent. The example shown only works when you hover the icon itself. Try to implement .ui-button:hover .ui-icon
, with a variable function for instance. You'd be doing this:
const buttonVariables = (siteVariables, props) => {
icon: {
// !
// ! Where did props.isHovering come from here?
// !
color: props.isHovering ? 'red' : 'green'
}
}
How do you know the button is being hovered from within the context of the variables function? The only input is the props, which must come from state. There is no way to use a CSS selector for this.
applyIconStyles()
I wouldn't create a function for this. Teams theme knows that its icons are SVG, so it applies icon color to the fill
prop (though we should ideally support fonts and svgs anyway). Whether it does this in the icon styles, radio styles, or button styles, etc. makes no difference. It is a theme concept, not an icon component concept.
const iconStyles = {
root: ({ variables }) => ({
color: variables.color, // handle fonts
fill: variables.color, // handle svgs
})
}
const radioStyles = {
root: ({ variables }) => ({
':hover': {
'.ui-icon': {
color: variables.iconHoverColor, // handle fonts
fill: variables.iconHoverColor, // handle svgs
}
}
})
}
If it is OK to code icon style logic in one part of the Teams theme, then I think it is acceptable to code it in other parts.
Agree on most of the parts. Let me, please, summarise on where we are at the moment.
Essentially we've come up with the following problem that we would like to find solution for:
Suppose that we have a child component inside other one (parent). How we could provide styling for this child component in case if pseudo state of parent has changed. Specific example: how to tweak styles of child
Icon
when hover state of parentRadio
component was changed?
What we've agreed upon is that there are, actually, two main approaches to solve this task:
.ui-radio:hover .ui-icon {
// styles for Icon of hovered Radio
...
}
hover
state of Radio
in JS world and translate this knowledge to Radio
's component state. After that task of styling Icon
becomes to be rather trivial, as it reduces to projecting this state to Icon's style:// radioStyles
() => {
icon: ({ props: radioProps /* note that these are Radio's props */ }) => ({
color: radioProps.hover ? 'red' : 'blue'
})
Major benefit of this approach would be the fact that we will continue to rely on CSS power while doing this styling - specifically
However, here is a problem that is associated with this approach - with current Stardust logic we are not able to transform component style objects to complex selectors like the following one, and this is a problem we need to solve then.
.parent-component:hover child-component {
...
}
Major benefit of this approach is that we would need to think about less problems to solve - essentially, the only one: how we could reliably detect pseudo state for a component, so it could be projected to component's JS state.
But this task turns out to be very hard to solve - and this fact leads to the following problems associated with this approach:
:hover
state is to subscribe component on mouse events like onMouseEnter
and onMouseLeave
, and do corresponding state changes there. Although it might seem that this approach will solve the problem for a component, there are additional things that we would need to ensure afterwards:
:hover
we might consider to use onMouseEnter
and onMouseLeave
events, but for other ones the logic might differ - and for each case we will need to solve the same set of problems like 'how reliable it is' and 'how it affects performance'At the same time, I am not completely agree that we should end up with the approach that will explicitly suggest CSS styles to child (slot) component, like the following:
const radioStyles = {
root: ({ variables }) => ({
':hover': {
// WARNING
// !! note that here we are implicitly coupling style aspects of child component
// with the ones of parent !!
'.ui-icon': {
color: variables.iconHoverColor, // handle fonts
fill: variables.iconHoverColor, // handle svgs
}
}
})
}
The reason, once again, is that with this we won't be able to advertise these slots as something that could host any component - as we will, essentially, couple styles of specific child component with styles of its parent.
This is, essentially, the reason I do see this problem should be solved using 'language' of variables, i.e. on the higher level of abstraction.
Still, lets focus on addressing the problem with pseudo-state styles first, and I am pretty sure that we would be able to improve on that afterwards.
I think there are good cases to remove this. Let's have a discussion in one of our arch meets.
agree, this is an obsolete issue - currently the problem that has been raised is resolved. All the styling aspects (i.e. both styles and variables) are now driven by theme author, so there is nothing that is built into core Stardust logic. Closing
Feature Request
Where we are now
Agreed on the following task (https://github.com/stardust-ui/react/issues/231#issuecomment-421973984) to be solved first by trying to prototype each of the suggested approaches. Here are prototype links to try:
Problem description
After syncing up with Fabric there were some concerns raised around our taken approach to use
props
as variables function' argument:Proposed solution
Although a step related to make styling aspects to be completely a theme's area of responsibility is definitely a step in the right direction, there is need to discuss potential alternatives to proposed solution.
Please, consider this PR where discussion thread has started #207 - all related posts are cited here as well.