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

refactor(Divider): remove type prop #558

Merged
merged 9 commits into from
Dec 6, 2018
Merged

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Dec 5, 2018

Fixes #421.

BREAKING CHANGES

We decided to drop type prop on this component because type is a reserved prop in HTML.

Before

<Divider type='primary' />
<Divider type='secondary' />

After

<Divider color='primary' />
<Divider color='secondary' />

` }}`,
` secondary`,
` styles={styles} />`,
` />`,
Copy link
Member Author

Choose a reason for hiding this comment

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

I aligned code in these examples.

Before

image

After

image

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

@layershifter I just merged prettier integration #568 which auto formats CodeSnippets and ExampleSnippets. It also removes the need to have the value string prop altogether by printing the render prop return value and prettifying it 👍

Just fix conflicts here to account for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how would this reflect the fact that we want to show the examples as Javascript code, but we are writing the actual rendered code in Typescript... I have something like this in the Integrate Custom Components guide. What is the proposal for addressing this?

@layershifter
Copy link
Member Author

I decided to remove Divider from theming examples because we using color palette there and examples will be overcomplicated (current and previous pattern).

After we will apply color palette to all our components we will return Divider.

@layershifter layershifter changed the title [WIP] breaking(Divider): remove type prop breaking(Divider): remove type prop Dec 5, 2018
@layershifter layershifter changed the title breaking(Divider): remove type prop refactor(Divider): remove type prop Dec 5, 2018
import { EmphasisColors, NaturalColors } from '../../../types'
import { pxToRem } from '../../../../lib'

export interface DividerVariables {
colors: EmphasisColors & NaturalColors
colors: Record<keyof (EmphasisColors & NaturalColors), string>
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

TIL: Record

importantFontWeight: string
dividerPadding: string
}

export default (siteVars: any): DividerVariables => {
return {
colors: { ...siteVars.emphasisColors, ...siteVars.naturalColors },
colors: _.mapValues({ ...siteVars.emphasisColors, ...siteVars.naturalColors }, '500'),
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the magic '500' doing here? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

We have colors with following structure:

const colors = {
  blue: { 500: 'blue' // other values },
  pink: { 500: 'pink'  },
}

I decided to reduce this structure with mapValues to as we using only single variant:

const colors = {
  blue: 'blue',
  pink: 'pink',
}

This structure is more usable for overrides, too.
Yes, it looks like a magic number, but it is a simply color variant.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can do this:

const colorVariant = '500'

return {
  colors: _.mapValues({ ...siteVars.emphasisColors, ...siteVars.naturalColors }, colorVariant),
}

May be, this change will improve readability.

Copy link
Member

Choose a reason for hiding this comment

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

We can save my comment here for another PR, but here are some thoughts.


Wondering if we should go this route, defining single color variable for every component. Most components will implement the same variant of a color for the same purpose. Example, a blue border will be the same shade in components that have blue borders. Red text is similar where ever it appears. The odds that a specific component needs to override a specific shade of color I think would be the exception and not the norm.

We might be able to produce a "scheme" for a component to implement. Imagine picking colors along a color object like this:

const primary = {
  50: '#F4F4FC',  // background
  100: '#E2E2F6', // shadow
  200: '#BDBDE6', // border
  300: '#8F90C1',
  400: '#6E70AE',
  500: '#6264A7', // text
  600: '#55578D',
  700: '#4A4C78',
  800: '#414265',
  900: '#33344A',
}

image

Or, an inverted version of the same component using different variants from the same object:

const primary = {
  50: '#F4F4FC',  // text
  100: '#E2E2F6',
  200: '#BDBDE6', // shadow
  300: '#8F90C1',
  400: '#6E70AE',
  500: '#6264A7', // background
  600: '#55578D',
  700: '#4A4C78', // border
  800: '#414265',
  900: '#33344A', 
}

image

Divider

Applied to the Divider, I can imagine variables for the borderColor and a textColor since these are the relevant parts of the component and can have different colors, such as here:

image

However, I think what is missing in this PR right now is that there is no indication of which variant the text color will use compared to the border variant. We might want a way to choose a "scheme" for applying to a component or at least specify which variants will be used.

Schemes

If we try the scheme route, we might look at many components in a UI and ask what the common color schemes are. A scheme would includes colors for text, background, border, and shadow.

I can see defining a "primary" scheme or a "secondary" scheme, even "inverted" schemes in this way. The scheme from the first Message looking component above would look something like this:

const scheme = {
  foreground: primary[500],

  quiet: primary[400],         // range of "muted" foreground colors
  quieter primary[300],
  quietest: primary[200],

  background: primary[50],
  border: primary[200],
  shadow: primary[100]
}

These could be generated for any color, inversion, emphasis, and state like disabled. They could also be adjusted for higher/lower contrast or brightness.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's a long discussion :) Not for this PR, that's why I removed Divider from examples of theming.

Copy link
Contributor

Choose a reason for hiding this comment

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

@layershifter yep, please introduce the changes proposed in #558 (comment) it is much clear that way.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

👍

@layershifter layershifter merged commit 8a3fbed into master Dec 6, 2018
@layershifter layershifter deleted the breaking/divider branch December 6, 2018 13:12
import { EmphasisColors, NaturalColors } from '../../../types'
import { pxToRem } from '../../../../lib'

export interface DividerVariables {
colors: EmphasisColors & NaturalColors
colors: Record<keyof (EmphasisColors & NaturalColors), string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@layershifter can we extract this interface
Record<keyof (EmphasisColors & NaturalColors), string>
in src/themes/types.ts to reuse it for other's components color prop?

return {
colors: { ...siteVars.emphasisColors, ...siteVars.naturalColors },
colors: _.mapValues({ ...siteVars.emphasisColors, ...siteVars.naturalColors }, colorVariant),
Copy link
Collaborator

Choose a reason for hiding this comment

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

@layershifter can we extract this method
_.mapValues({ ...siteVars.emphasisColors, ...siteVars.naturalColors }, colorVariant),
or even an object if the colorVariant is always 500
export themeColors = _.mapValues({ ...siteVars.emphasisColors, ...siteVars.naturalColors }, 500)
to reuse it for other's components color prop?

This was referenced Dec 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants