Checkbox base prototype

[joschect]

Base Component Prototype

Purpose

The purpose of this PR is to act as a proof of concept for implementing base components which will underly the full "fluent" components. The goal is to be as framework agnostic as possible. It provides an api surface that is decoupled from styling, theming, and composition concerns while still allowing those to be implemented.

[khmakoto]

You might want to pull this out of the render function in a const object so that this isn't recomputed on every render.

[khmakoto]

Will these types be more refined in the final version?

[khmakoto]

Maybe TestHelper?

[khmakoto]

You could fuse this two ifs into one.

[khmakoto]

Do we need to add setIsChecked here? From my understanding, since the function will never change its value, it is a variable that will never trigger a different callback.

[khmakoto]

I'm having a really hard time figuring out why we need this instead of using the regular isChecked.

[khmakoto]

Suggested change

	describe("Baby's first test", () => {
	describe("CheckboxBase", () => {

[levithomason]

Let's organize our tests by:

describe(displayName, () => {
  describe(propName, () => {
    it('does something', () => {})
    it('does not do something', () => {})
  })
})

This pattern of Component > prop is used for all tests and docs. Following it ensures we can do a better of job of ensuring we have full coverage of documentation and tests. Having complete docs/tests on public APIs means we can also rev the library with more confidence and less bugs.

[khmakoto]

What's with the comment here? I don't understand it very well.

[levithomason]

Event handlers are tested as part of the baseline isConformant() test. This test checks every synthetic event type and asserts more things:

import { isConformant } from 'test/specs/commonTests'

describe('CheckboxBase', () => {
  isConformant(CheckboxBase)
})

See .github/test-a-feature.md for more.

[khmakoto]

Wouldn't the keyCode have been enough here?

[levithomason]

@khmakoto see https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode, it's deprecated.

Only key is the valid check moving forward:

https://github.com/levithomason/keyboard-key#why

[khmakoto]

@levithomason sure, what I meant is that having both was redundant, I don't actually have a preference for one over the other.

[khmakoto]

Suggested change

	it('renders classes', () => {
	it('renders classes for root', () => {

[jurokapsiar]

is it still valid that the state will need to be pulled out of the base component?

[jurokapsiar]

according to ARIA only space should toggle the checked state. How would we allow customization (if some partner wants to toggle on enter as well)? Would that need to be a separate or a custom component?

[jurokapsiar]

In both Semantic UI React and Stardust there is the concept of auto controlled state which allows the consumer to take over and control the state from outside by using a combination of the chnge handler and override state prop. It is especially useful in components like Popup or Dialog where the consumer can add control if the surface should be opened/closed initially or based on external event, as well as additional logic to extend the internal handlers. Maybe this is a simplified version of the API, but it would be good to keep it as it turned out to be very effective and consistent way of controlling the state from outside.

[levithomason]

Small note, the concept of "auto controlled state" comes from React itself. Form controls, like the <input />, will automatically control their value when the user inputs values. However, if the dev controls the value prop, then it will defer to the dev's props. If the dev removes the prop, then React resumes control of the <input />.

The principles that follow from this is that:

1. Components should work without requiring the consumer to wire them up. Dropdowns should open on click, inputs should update their value on input, etc.
2. Components should defer to controlled mode if the dev provides props
3. Components should resume control if the developer "unsets" the prop value (i.e. undefined).

[jurokapsiar]

should this be called onClickHandlefr?

[levithomason]

Convention is handleClick, this is a common community convention based on popular libs and the React docs:

https://reactjs.org/docs/handling-events.html

In React, this could instead be:

function ActionLink() {
  function handleClick(e) {
    e.preventDefault();
    console.log('The link was clicked.');
  }

  return (
    <a href="#" onClick={handleClick}>
      Click me
    </a>
  );
}```