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

[RFC] feat(Divider): add color prop #436

Closed
wants to merge 3 commits into from
Closed

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Nov 6, 2018

Fixes #423.
Refs #421, #426.

This PR adds the color prop for the Divider component.

Blocked By

TODO

  • Cover getColor() with tests
  • Minimal doc site example
  • Update the CHANGELOG.md
  • Define correct colors in the palette

API Proposal

ColorPalette

I took the SUI palette as reference:

interface ColorPalette {
  primary: string
  secondary: string

  black: string
  brown: string
  blue: string
  green: string
  gray: string
  olive: string
  orange: string
  pink: string
  purple: string
  teal: string
  red: string
  violet: string
  white: string
  yellow: string
}

I have doubts about primary and secondary colors, the code below looks very confusing:

<Button color='primary' primary />

ComponentColors

A type that we will be used in props, defines all possible names of colors and includes string.

color

The color prop accepts colors from the palette and custom colors.

image

  <Divider color='black' content="Black" />
  <Divider color='#000' content="Black" /> // still black!
  <Divider color='rgb(192, 192, 192)' content="Gray" /> // and even RGB

However, I'm also not sure about custom colors. It plays well semantically, however we have variables for this:

image

  <Divider color='pink' content="Pink" variables={{ colors: { pink: 'pink' }}} />

@codecov
Copy link

codecov bot commented Nov 6, 2018

Codecov Report

Merging #436 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
+ Coverage   91.84%   91.85%   +<.01%     
==========================================
  Files          41       41              
  Lines        1337     1338       +1     
  Branches      193      168      -25     
==========================================
+ Hits         1228     1229       +1     
  Misses        105      105              
  Partials        4        4
Impacted Files Coverage Δ
src/components/Divider/Divider.tsx 100% <ø> (ø) ⬆️
src/index.ts 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d91db4...e4a83d9. Read the comment docs.

@layershifter layershifter changed the title [RFC][WIP] feat(Divider): add color prop [RFC] feat(Divider): add color prop Nov 6, 2018
@mnajdova
Copy link
Contributor

mnajdova commented Nov 6, 2018

I like the implementing, and vote against using the primary as secondary as a color, as those are abstraction for something more then a color, that's why we have them as a properties.

Copy link
Member

@miroslavstastny miroslavstastny left a comment

Choose a reason for hiding this comment

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

I think color palette (list of color names) should not be part of the API, but should be defined by a theme. Different themes should be able to define different list of colors.

If that's the case, colors cannot be enumerated in PropTypes, examples should read the list from current theme (see TextSizesExampleShorthand as an example).

Other opinions? @levithomason

@layershifter
Copy link
Member Author

Different themes should be able to define different list of colors.

We can define only base colors (i.e. black, white, blue, gray, etc) and leave colors extendable, so end users will be able to define own colors.

@layershifter
Copy link
Member Author

Blocked by #423.

@layershifter
Copy link
Member Author

Will be done as part of #451 to confirm that palette ideas are working.

@layershifter layershifter deleted the feat/color-palette branch November 22, 2018 16:02
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.

3 participants