timelessco / chakra-components

Components built on top of Chakra UI
MIT License
1 stars 0 forks source link

[RFC] Checkbox Component #16

Closed navin-moorthy closed 4 years ago

navin-moorthy commented 4 years ago

Introduction

We will start writing the basic Checkbox from Chakra UI and start building on this later one by one.

Checkbox to be built

image

Initial Steps

Props to be exposed

Code collaboration Flow

We will improve the steps based on our discussion. First, we need to begin with coding. At last, we can add accessibility styles.

prasanna1211 commented 4 years ago

What are the props that we are going to provide ?

navin-moorthy commented 4 years ago

What are the props that we are going to provide ?

We will eventually provide props, first, let's get started with just wrapping around the Checkbox component and improve step by step.

SamrithaS commented 4 years ago

my thoughts on what props could be are:

thoughts on this? @prasanna1211 @navin-moorthy @govindsingh55

navin-moorthy commented 4 years ago

my thoughts on what props could be are:

  • color of the checkbox
  • icon of the checkbox
  • number/names of items that are selected
  • onClick / onSelected function
  • styles like bg, fontColor, fontSize for selected state and unselected ones

thoughts on this? @prasanna1211 @navin-moorthy @govindsingh55

yeah, we will add all these props after deciding on which value all these props will take.

prasanna1211 commented 4 years ago

@SamrithaS can you elaborate on Color ? Background or border ? Icon ? icon before (or) after. Also why a checkbox needs an icon ? Checkbox always has only one state. Selected / not selected. Only checkbox group has selected items. Difference between onClick and onSelect ? For style there is something called as styled props for chakra. Can you refer to it ?

prasanna1211 commented 4 years ago

Existing props are in here: https://chakra-ui.com/checkbox, may be compiling from this becomes easy.

navin-moorthy commented 4 years ago

Existing props are in here: https://chakra-ui.com/checkbox, may be compiling from this becomes easy.

Yeah, I will start off with a new branch and the component to refer and by reviewing it we all can get an idea on what we are going to do with Checkbox.

SamrithaS commented 4 years ago

@SamrithaS can you elaborate on Color ? Background or border ? Icon ? icon before (or) after. Also why a checkbox needs an icon ? Checkbox always has only one state. Selected / not selected. Only checkbox group has selected items. Difference between onClick and onSelect ? For style there is something called as styled props for chakra. Can you refer to it ?

prasanna1211 commented 4 years ago

@SamrithaS you can look into this https://chakra-ui.com/checkbox too. Basically what we are going to do is, for our checkbox component we are going to add few styling and missing functionalities like effects / animations and required props. So that when we render just it comes out with a pleasing styles.

navin-moorthy commented 4 years ago

@govindsingh55 @SamrithaS Let's first do not expose any prop from our component. We will discuss and fix the base styles from the sketch file and create the base component.

In the example on above PR, I created a red varient Checkox, which we are going to change to match the app styles.

Any thoughts?

SamrithaS commented 4 years ago

@govindsingh55 @SamrithaS Let's first do not expose any prop from our component. We will discuss and fix the base styles from the sketch file and create the base component.

In the example on above PR, I created a red varient Checkox, which we are going to change to match the app styles.

Any thoughts?

yes, we can proceed by matching the styles

navin-moorthy commented 4 years ago

@govindsingh55 @SamrithaS You can pull this and continue working on this component based on our decisions. Next thing, we can include

image

variantColor = #31a9d8 to our checkbox.

In order to add the variant color, we need to create our own colors object in the name of "brand" in theme.js and import it to our ThemeProvider.

Create a multi color shade to support variantColor prop

import { theme } from '@chakra-ui/core';

export default {
  ...theme,
  colors: {
    ...theme.colors,
    brand: {
      '50': '#8ED0E9',
      '100': '#7BC8E6',
      '200': '#69C0E2',
      '300': '#56B8DF',
      '400': '#43B0DB',
      '500': '#31A9D8',
      '600': '#2D9AC5',
      '700': '#298BB1',
      '800': '#247B9E',
      '900': '#206C8A',
    },
  },
};
navin-moorthy commented 4 years ago

Anyone can find a way to add Inter font-family to our project and since our app requires it. Also, comment on how you are going to implement it.

SamrithaS commented 4 years ago

@govindsingh55 @SamrithaS You can pull this and continue working on this component based on our decisions. Next thing, we can include

image

variantColor = #5D5E5E to our checkbox.

In order to add the variant color, we need to create our own colors object in the name of "brand" in theme.js and import it to our ThemeProvider.

I'll do it

navin-moorthy commented 4 years ago

Anyone can find a way to add Inter font-family to our project and since our app requires it. Also, comment on how you are going to implement it.

@govindsingh55 Please take this and let us know your thoughts.

navin-moorthy commented 4 years ago

Also we need to find a way to over-write the default checkbox border to match the below style when not checked.

Default chakra style,

Also we need to find a way to over-write the default checkbox border to match the below style when not checked.

Default chakra style, image

Our component style, image

Any suggestion?

@govindsingh55 @SamrithaS You can pull this and continue working on this component based on our decisions. Next thing, we can include image

variantColor = #5D5E5E to our checkbox.

In order to add the variant color, we need to create our own colors object in the name of "brand" in theme.js and import it to our ThemeProvider.

I'll do it

After doing it, make a commitizen commit and let us know, if you face any problem we will help you out.

SamrithaS commented 4 years ago

sure, I'll let you know

navin-moorthy commented 4 years ago

Anyone can find a way to add Inter font-family to our project and since our app requires it. Also, comment on how you are going to implement it.

@govindsingh55 Please take this and let us know your thoughts.

Since @govindsingh55 is away, I will take this and proceed with this implementation steps.

navin-moorthy commented 4 years ago

In order to create our own custom checkbox, we will need to refractor our use of Chakra Checkbox to use Chakra Control Box

 fonts: {
    ...theme.fonts,
    heading: `Inter,${theme.fonts.heading}`,
    body: `Inter,${theme.fonts.body}`
  },

All the below typography styles are done best to match our app styles.

<Box lineHeight="14px">
    <label style={{ lineHeight: 'inherit' }}>
      {/* This is the sibling input, it's visually hidden */}
      <VisuallyHidden as="input" type="checkbox" defaultChecked />

      {/* This is the control box with a check icon as children */}
      <ControlBox
        borderWidth="1.5px"
        size="14px"
        rounded="sm"
        _checked={{ bg: 'brand.500', color: 'white', borderColor: 'brand.500' }}
      >
        <Icon name="check" size="10px" />
      </ControlBox>

      {/* You can pass additional text */}
      <Box
        as="span"
        verticalAlign="top"
        ml={3}
        fontFamily="body"
        fontSize="13px"
        fontWeight="medium"
        color="#333536"
        letterSpacing="0.13px"
      >
        {children}
      </Box>
    </label>
  </Box>

Let me know, if we need to change anything or improve on the above plan.

navin-moorthy commented 4 years ago

Also we need to find a way to over-write the default checkbox border to match the below style when not checked.

Default chakra style,

Also we need to find a way to over-write the default checkbox border to match the below style when not checked.

Default chakra style, image

Our component style, image

Any suggestion?

@SamrithaS Look at the sketch file to find border styles - border color, border width, border radius Also look at the chakra [Control Box] control box to find a way to change our Checkbox styles like the steps above.

SamrithaS commented 4 years ago

working on the border, could you tell me if this colour #919494 should be fine?

navin-moorthy commented 4 years ago

working on the border, could you tell me if this colour #919494 should be fine?

Since you don't have sketch, I will say the specifications,

borderColor - #8C8D8E borderWidth - 1.5px borderRadius - Not accurate, lets fix at 25% as per sketch

SamrithaS commented 4 years ago

working on the border, could you tell me if this colour #919494 should be fine?

Since you don't have sketch, I will say the specifications,

borderColor - #8C8D8E borderWidth - 1.5px borderRadius - Not accurate, lets fix at 25% as per sketch

great, thanks

navin-moorthy commented 4 years ago

@govindsingh55 Kindly work implementation step for adding check icon from our sketch file to our Checkbox using theme.js and change the chakra icon since it is different one.

navin-moorthy commented 4 years ago

After adding the icon, we will finalize the hove, focus & active styles. Once all the styles are done, we can work on the props one by one based on the difficulty

SamrithaS commented 4 years ago

Steps to add the Border styles:

theme.js contains: checkbox: { borderColor: '#8C8D8E', },

and Checkbox/index.js contains: ` <ControlBox borderWidth="1.5px" size="14px" borderColor="checkbox.borderColor" rounded="md" _checked={{ bg: 'brand.500', color: 'white', borderColor: 'brand.500' }}

`

navin-moorthy commented 4 years ago

Based on research, it seems our checkbox doesn't need any active and hover styles. So we will be only adding the focus styles _focus={{ boxShadow: 'outline' }}

Any thoughts?

navin-moorthy commented 4 years ago

Just now noticed in the sketch file, we need to change the ml for our Children Box should be ml={2} instead of 3

SamrithaS commented 4 years ago

Just now noticed in the sketch file, we need to change the ml for our Children Box should be ml={2} instead of 3

I'll do it

navin-moorthy commented 4 years ago

@SamrithaS We can decide on all the props that we are going to expose from our component since @govindsingh55 is now working on adding the Icon.

SamrithaS commented 4 years ago

okay, what's your suggestion on the props?

navin-moorthy commented 4 years ago

Initial thoughts for checkbox props are,

navin-moorthy commented 4 years ago

Any other props you feel, we can add it to the top description based on your discussion

SamrithaS commented 4 years ago

Any other props you feel, we can add it to the top description based on your discussion

This seems fine, let's start working on it, and add later if we need more

navin-moorthy commented 4 years ago

Sure, can you work on adding disabled first and I will say the styles for that.

SamrithaS commented 4 years ago

Sure, can you work on adding disabled first and I will say the styles for that.

sure

navin-moorthy commented 4 years ago

Seems there is hover style for our checkbox, I will work on adding it. image

Add Hover styles as

_hover={{
          borderColor: 'brand.500',
          shadow: '0 0 0 2px #31a7d840, inset 0 0 0 2px #31a7d840',
        }}
navin-moorthy commented 4 years ago

Based on the comment from Sandeep, we are going to change our Inter Font Family from primary one to Google Fonts which is more performant

<link href="https://fonts.googleapis.com/css?family=Inter:100,200,300,400,500,600,700,800,900&display=swap" rel="stylesheet">

navin-moorthy commented 4 years ago

@govindsingh55 Please add the steps on how you are implementing, so that we all can agree and do the commit. This will be useful for issue reference

SamrithaS commented 4 years ago

@navin-moorthy Can you tell me the styles for disabled?

navin-moorthy commented 4 years ago

@SamrithaS the disable state - borderColor: "#E7E8E9" fontColor: "#CACBCC"

navin-moorthy commented 4 years ago

I will proceed to add the Icon only to our checkbox and decide on the props later,


  icons: {
    ...theme.icons,
    customCheck: {
      path: (
        <path
          fill="none"
          fillRule="evenodd"
          stroke="#FFF"
          strokeLinecap="round"
          strokeLinejoin="round"
          strokeWidth="1.5"
          d="M1.5 3.788L3.651 6l5.058-5"
        />
      ),
      viewBox: '0 0 10 7',
    },
  },

We can add the props after deciding.

navin-moorthy commented 4 years ago

I have listed all props that we are going to expose as per Sandeep's comment

Make sure we've all the existing props from chakra surfaced.

We can add additional props anytime.

SamrithaS commented 4 years ago

steps for disabled props:

Homepage/index.js contains

<Box p="4">
        <Checkbox isDisabled>Three</Checkbox>
      </Box>

checkbox/index.js contains

`const Checkbox = ({ children, isDisabled }) => (
  <Box lineHeight="14px">
    <label style={{ lineHeight: 'inherit' }}>
      {/* This is the sibling input, it's visually hidden */}
      <VisuallyHidden as="input" type="checkbox" />

      {/* This is the control box with a check icon as children */}
      <ControlBox
        borderWidth="1.5px"
        size="14px"
        borderColor={isDisabled ? '#E7E8E9' : 'checkbox.borderColor'}
        rounded="md"
        _checked={
          isDisabled
            ? {}
            : { bg: 'brand.500', color: 'white', borderColor: 'brand.500' }
        }
        _focus={{ borderColor: 'outline' }}
        _hover={
          isDisabled
            ? {}
            : {
              borderColor: 'brand.500',
              shadow: '0 0 0 2px #31a7d840, inset 0 0 0 2px #31a7d840',
            }
        }
      >
        <Icon name="customCheck" size="10px" opacity={isDisabled ? 0 : 100} />
      </ControlBox>

      {/* You can pass additional text */}
      <Box
        as="span"
        ml={2}
        fontFamily="body"
        fontSize="13px"
        fontWeight="medium"
        color={isDisabled ? '#CACBCC' : '#333536'}
        letterSpacing="0.13px"
      >
        {children}
      </Box>
    </label>
  </Box>
);

Checkbox.propTypes = {
  children: PropTypes.string,
  isDisabled: PropTypes.bool,
};

export default Checkbox;
`
govindsingh55 commented 4 years ago

steps for disabled props:

Homepage/index.js contains

<Box p="4">
        <Checkbox isDisabled>Three</Checkbox>
      </Box>

checkbox/index.js contains

`const Checkbox = ({ children, isDisabled }) => (
  <Box lineHeight="14px">
    <label style={{ lineHeight: 'inherit' }}>
      {/* This is the sibling input, it's visually hidden */}
      <VisuallyHidden as="input" type="checkbox" />

      {/* This is the control box with a check icon as children */}
      <ControlBox
        borderWidth="1.5px"
        size="14px"
        borderColor={isDisabled ? '#E7E8E9' : 'checkbox.borderColor'}
        rounded="md"
        _checked={
          isDisabled
            ? {}
            : { bg: 'brand.500', color: 'white', borderColor: 'brand.500' }
        }
        _focus={{ borderColor: 'outline' }}
        _hover={
          isDisabled
            ? {}
            : {
              borderColor: 'brand.500',
              shadow: '0 0 0 2px #31a7d840, inset 0 0 0 2px #31a7d840',
            }
        }
      >
        <Icon name="customCheck" size="10px" opacity={isDisabled ? 0 : 100} />
      </ControlBox>

      {/* You can pass additional text */}
      <Box
        as="span"
        ml={2}
        fontFamily="body"
        fontSize="13px"
        fontWeight="medium"
        color={isDisabled ? '#CACBCC' : '#333536'}
        letterSpacing="0.13px"
      >
        {children}
      </Box>
    </label>
  </Box>
);

Checkbox.propTypes = {
  children: PropTypes.string,
  isDisabled: PropTypes.bool,
};

export default Checkbox;
`

style is messing a bit check svg is being visible try to run it

navin-moorthy commented 4 years ago

Add the cursor = not allowed when isDisabled

navin-moorthy commented 4 years ago

style is messing a bit check svg is being visible try to run it

For me, everything looks fine, check svg is not visible after passing isDisabled props. Can you confirm agian.

image

SamrithaS commented 4 years ago

Add the cursor = not allowed when isDisabled

I'll do it

navin-moorthy commented 4 years ago

Doesn't any one get this linting error, while reproducing the above code?

image

Seems to be a conflict between Eslint and Prettier rules.

govindsingh55 commented 4 years ago

style is messing a bit check svg is being visible try to run it

For me, everything looks fine, check svg is not visible after passing isDisabled props. Can you confirm agian.

image

see the white correct tick is bit visible.

on isDisabled = false , add cursor to the