Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix(Button): icon colors and layout #135

Merged
levithomason merged 11 commits into from
Aug 30, 2018
Merged

fix(Button): icon colors and layout #135

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7689f77
fix layuot and icon color of Button
kuzhelov-ms Aug 24, 2018
f81b239
fix color stylings of Button
kuzhelov-ms Aug 24, 2018
aeded91
fix children API
kuzhelov-ms Aug 30, 2018
05fe3fc
align content to center
kuzhelov-ms Aug 30, 2018
66dab28
remove unused 'content' prop from styles
kuzhelov-ms Aug 30, 2018
cc35644
fix color applied for hover state
kuzhelov-ms Aug 30, 2018
7627b6c
fix child API
kuzhelov-ms Aug 30, 2018
a048e8e
use site variable to initialize default color
kuzhelov-ms Aug 30, 2018
a4f5d32
Merge branch 'master' into fix/button-layout-and-icon-color
kuzhelov-ms Aug 30, 2018
94a0606
changelog
kuzhelov-ms Aug 30, 2018
c1d5714
Merge branch 'master' into fix/button-layout-and-icon-color
levithomason Aug 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 23 additions & 7 deletions src/components/Button/Button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,20 @@ class Button extends UIComponent<any, any> {
accessibility: ButtonBehavior as Accessibility,
}

public renderComponent({ ElementType, classes, accessibility, rest }): React.ReactNode {
public renderComponent({
ElementType,
classes,
accessibility,
variables,
rest,
}): React.ReactNode {
const { children, content, disabled, iconPosition } = this.props
const hasChildren = childrenExist(children)

if (hasChildren) {
return children
}

@levithomason levithomason Aug 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If I now do:

<Button>Click me</Button>

Won't this result in rendering the text "Click me" only? We'd still want to wrap the text in an ElementType and apply our classes/accessibility/rest props.


return (
<ElementType
className={classes.root}
Expand All @@ -104,21 +114,27 @@ class Button extends UIComponent<any, any> {
{...accessibility.attributes.root}
{...rest}
>
{hasChildren && children}
{!hasChildren && iconPosition !== 'after' && this.renderIcon()}
{!hasChildren && content}
{!hasChildren && iconPosition === 'after' && this.renderIcon()}
{iconPosition !== 'after' && this.renderIcon(variables)}
{content && <span className={classes.content}>{content}</span>}
{iconPosition === 'after' && this.renderIcon(variables)}
</ElementType>
)
}

public renderIcon = () => {
public renderIcon = variables => {
const { disabled, icon, iconPosition, content, type } = this.props

return Icon.create(icon, {
defaultProps: {
xSpacing: !content ? 'none' : iconPosition === 'after' ? 'before' : 'after',
variables: { color: type === 'primary' && !disabled ? 'white' : undefined },
variables: {
color:
type === 'primary'
? variables.typePrimaryColor
: type === 'secondary'
? variables.typeSecondaryColor
: variables.color,
},

@levithomason levithomason Aug 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any variables or styles used directly in a component's logic implicitly become part of a global theming interface.

All themes will have to implement variables.typePrimaryColor, for instance, in order for this to work. Additionally, we are removing the styling from the theme and baking it into the component instead.

I think all theming values and theming logic should live in the theme's files. This way, each theme is free to choose the values and logic behind their styles. The question is how?

Proposal 1 - Component part variables

Component variables files could define variables for each component parts as well. Then, variable values and logic for component parts can be left to the theme and not dictated to all themes by the component. Here's an example of the Button theme defining an icon part in its variables.

// themes/teams/components/Button/buttonVariables.tsx

export default (siteVariables) => {
  const variables = {
    color: undefined,
    typePrimaryColor: siteVariables.white,
    typeSecondaryColor: siteVariables.black,
  }

  variables.icon = {
    color:
      type === "primary"
        ? variables.typePrimaryColor
        : type === "secondary"
          ? variables.typeSecondaryColor
          : variables.color
  }

  return variables
}

Now, the component simply implements a contract that says "we pass variables[part] to each component part":

// Button.tsx

Icon.create(icon, {
  defaultProps: {
    variables: variables.icon
  }
})

This would also allow us to do this consistently for all components, including the styles for each component part. That then would allow us to write a conformance test for it. This would make the API easier to understand and work with as well since there is only one concept, "components pass variables/styles to each component part".

Proposal 2 - A high-level theme interface

This proposal would introduce a new concept. Something like a lightweight theme interface that is allowed to pass between components. It would consist of backgroundColor, foregroundColor, and perhaps an accentColor initially (names and keys are inspired by present convergence talks with several teams at Microsoft).

The Button in this case would define these three values for its bounds. Then, they would passed to the Icon.

There are a lot more considerations here, but I'm less in favor of this pattern for the problem at hand so I'll leave it here for now.

Thoughts?

@kuzhelov kuzhelov Aug 30, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, agree with the reasons provided above, thanks - this goes in line with the exactly the same discussion we've had with Miro. For the sake of avoiding to introduce additional complexity at that moment we've decided to move forward with the simplest approach that would indicate our intent to pass variables to child component, and after that introduce changes in a way that theming aspects will be completely separated from logic.

Speaking of the options suggested - I would definitely support the first one, as it suggest clear path that not changes but extends approach we are using now - and, thus, it is not associated with significant complexity increase. Also, with this one we will be able to address quite broad set of scenarios we are currently struggling with (or using the approach where variables are coupled with implementation)

Not saying 'no' to the second option, though - but would rather wait once this concept will solidify in our minds, for the sake of us being absolutely sure about what we are buying for additional efforts laid in this direction, as well as whether this concept is able to cover the cases we are interested in (and how flexible it would be to cover potentially emerged edge-cases)

@kuzhelov kuzhelov Aug 30, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

for the sake of progressing with that, let me suggest the following steps:

  • leave the logic as it is for this PR - as from the consumer perspective provided changes introduce several important fixes
  • introduce RFC for those concerns that were raised about how variables should be passed to child components ([RFC] Decouple variables passed to child components from parent component's logic #162 )
  • implement related changes by means of dedicated PR (and, as part of this work, also ensure that improved approach is used at all places where variables were passed in the component's logic)

This will allow us to progress with fixes, as well as split these subsequent changes so that it would be easier to review them.

Please, let me know what do you think about it. Thanks!

@levithomason levithomason Aug 30, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

agreed, let's merge this

},
})
}
Expand Down
86 changes: 44 additions & 42 deletions src/themes/teams/components/Button/buttonStyles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@ import { disabledStyle, truncateStyle } from '../../../../styles/customCSS'

const buttonStyles: IComponentPartStylesInput = {
root: ({ props, variables }: { props: any; variables: any }): ICSSInJSStyle => {
const { circular, disabled, fluid, icon, iconPosition, type } = props
const { circular, content, disabled, fluid, type } = props
const primary = type === 'primary'
const secondary = type === 'secondary'

const {
height,
minWidth,
maxWidth,
color,
backgroundColor,
backgroundColorHover,
circularRadius,
Expand All @@ -30,29 +31,46 @@ const buttonStyles: IComponentPartStylesInput = {
height,
minWidth,
maxWidth,
color,
backgroundColor,
display: 'inline-block',
display: 'inline-flex',
position: 'relative',
padding: `0 ${pxToRem(paddingLeftRightValue)}`,
margin: `0 ${pxToRem(8)} 0 0`,
verticalAlign: 'middle',
borderRadius: pxToRem(2),
borderWidth: 0,

...truncateStyle,
borderWidth: `${secondary ? (circular ? 1 : 2) : 0}px`,
cursor: 'pointer',
':hover': {
backgroundColor: backgroundColorHover,
},

...(icon &&
(iconPosition
? {
display: 'inline-flex',
justifyContent: 'center',
}
: {
minWidth: height,
padding: 0,
})),
...(primary && {
color: typePrimaryColor,
backgroundColor: typePrimaryBackgroundColor,
borderColor: typePrimaryBorderColor,
':hover': {
backgroundColor: typePrimaryBackgroundColorHover,
},
}),

...(secondary && {
color: typeSecondaryColor,
backgroundColor: typeSecondaryBackgroundColor,
borderColor: typeSecondaryBorderColor,
':hover': {
borderColor: 'transparent',
backgroundColor: typeSecondaryBackgroundColorHover,
},
}),

...(!content && {
justifyContent: 'center',
}),

...(circular && {
justifyContent: 'center',
minWidth: height,
padding: 0,
borderRadius: circularRadius,
Expand All @@ -63,36 +81,20 @@ const buttonStyles: IComponentPartStylesInput = {
maxWidth: '100%',
}),

...(disabled
? disabledStyle
: {
borderWidth: `${secondary ? (circular ? 1 : 2) : 0}px`,
cursor: 'pointer',
':hover': {
backgroundColor: backgroundColorHover,
},

...(primary && {
color: typePrimaryColor,
backgroundColor: typePrimaryBackgroundColor,
borderColor: typePrimaryBorderColor,
':hover': {
backgroundColor: typePrimaryBackgroundColorHover,
},
}),

...(secondary && {
color: typeSecondaryColor,
backgroundColor: typeSecondaryBackgroundColor,
borderColor: typeSecondaryBorderColor,
':hover': {
borderColor: 'transparent',
backgroundColor: typeSecondaryBackgroundColorHover,
},
}),
}),
...(disabled && {
...disabledStyle,
':hover': {
borderColor: undefined,
backgroundColor: undefined,
},
}),
}
},

content: ({ props }) => ({
overflow: 'hidden',
...(typeof props.content === 'string' && truncateStyle),
}),
}

export default buttonStyles
2 changes: 2 additions & 0 deletions src/themes/teams/components/Button/buttonVariables.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export interface IButtonVariables {
height: string
minWidth: string
maxWidth: string
color: string
backgroundColor: string
backgroundColorHover: string
circularRadius: string
Expand All @@ -25,6 +26,7 @@ export default (siteVars: any): IButtonVariables => {
height: pxToRem(32),
minWidth: pxToRem(96),
maxWidth: pxToRem(280),
color: undefined,

@levithomason levithomason Aug 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems strange to have an undefined variable that is also never set to a value. Is there not a siteVariable available for this value?

In either case, what is the motivation for this variable?

@kuzhelov kuzhelov Aug 30, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the motivation for this variable is to allow to customize Button's color (where 'color' comes in general sense, not something that is tied to corresponding CSS property only). This will open way for the following customization scenarios, as well as allow to customize this aspect of Button component in docs

<Button variables={{ color: 'red' }} ... />

Seems strange to have an undefined variable that is also never set to a value. Is there not a siteVariable available for this value?

Yes, let me initialise it with an appropriate site variable - the reason haven't done it before is because it was unclear to me at this moment which ones will be provided by teams theme

backgroundColor: siteVars.gray08,
backgroundColorHover: siteVars.gray06,
circularRadius: pxToRem(999),
Expand Down