-
Notifications
You must be signed in to change notification settings - Fork 6
[WIP] Button #9
base: master
Are you sure you want to change the base?
[WIP] Button #9
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
/** | ||
* Copyright IBM Corp. 2018, 2019 | ||
* | ||
* This source code is licensed under the Apache-2.0 license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
/** | ||
* Configuration for the Button component | ||
* @module button/demo | ||
*/ | ||
import startCase from 'lodash/startCase'; | ||
import { generate } from './generate'; | ||
|
||
/** | ||
* Creates a button template configuration for a variant | ||
* @returns {globalTypedefs.fractalDemo} fractal demo object | ||
*/ | ||
const variantButtons = function() { | ||
const variants = {}; | ||
|
||
Object.keys(this.selectors.variants).forEach(variant => { | ||
variants[variant] = { | ||
name: variant, | ||
label: startCase(variant), | ||
context: generate.apply(this, [ | ||
{ | ||
variant, | ||
}, | ||
]), | ||
}; | ||
}); | ||
|
||
return variants; | ||
}; | ||
|
||
/** | ||
* Modifier: Anchor | ||
* @returns {globalTypedefs.fractalDemo} fractal demo object | ||
*/ | ||
const anchorButton = function() { | ||
return { | ||
name: 'anchor', | ||
label: 'Anchor', | ||
context: generate.apply(this, [ | ||
{ | ||
element: 'a', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like using an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the PAL example. I'd like to keep this proof of concept focused as narrowly on the spec structure and functionality as possible, and less on the differences between Carbon implementations. That effort begins after the proof of concept. That's why I included this in the proof of concept scope:
Carbon React button component can render as either There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We obviously don't want to change what React does at this point...but if our goal is to focus on Testing only the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing carbon-components-x -y and -z is not in scope for this proof of concept. -x, -y, and -z all implement button and link components differently and that's okay (for now). If we focus our efforts on proving the component specification concept instead of "what is a perfect button", then we'll achieve the desired outcomes in #5. By taking a stable starting point (to prevent discussion like "what is a perfect button"), and if we can hit enough edge cases by going through these four components, we can achieve what we're after and then after that, spend the time locking down each component spec. With that in mind, I'd argue this is a good example of when a component would need to support multiple element types. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, we're not trying to create a perfect button by pushing back on anchor, its more about keeping If we were working from a user's standpoint, the typical user goes to carbondesignsystem.com for button, they'll see nothing about an anchor element. With that in mind, I think we may be in disagreement as to the point of the POC. What we thought we were figuring out:
If we were working from requirements, rather than using implemented code as a blueprint, this debate could easily be "what is expected according to the requirements?" Where we're coming from is that without considering a component's requirements we're going to be doomed to loops where we're essentially dealing with point-of-view and opinion rather than outcomes via reasoned deliberation. All that being said, I totally understand the need to work on various aspects of what code we're trying to put in place, and it makes sense to have a focused PR that doesn't include |
||
}, | ||
]), | ||
}; | ||
}; | ||
|
||
/** | ||
* Modifier: Small | ||
* @returns {globalTypedefs.fractalDemo} fractal demo object | ||
*/ | ||
const smallButton = function() { | ||
return { | ||
name: 'small', | ||
label: 'Small', | ||
context: generate.apply(this, [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
{ | ||
size: 'small', | ||
}, | ||
]), | ||
}; | ||
}; | ||
|
||
/** | ||
* State: Disabled | ||
* @returns {globalTypedefs.fractalDemo} fractal demo object | ||
*/ | ||
const disabledButton = function() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why make these functions when they don't take any parameters so it's a consistent return? Seems odd to me to define these as functions versus a constant variable and use unnecessary memory There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did this to bind I'm open to other ways to handle prefixing to avoid binding and applying functions. Suggestions? |
||
return { | ||
name: 'disabled', | ||
label: 'Disabled', | ||
context: generate.apply(this, [ | ||
{ | ||
disabled: true, | ||
}, | ||
]), | ||
}; | ||
}; | ||
|
||
export default function demo() { | ||
return { | ||
variants: variantButtons.apply(this), | ||
modifiers: { | ||
types: { | ||
anchor: anchorButton.apply(this), | ||
}, | ||
sizes: { | ||
small: smallButton.apply(this), | ||
}, | ||
}, | ||
states: { | ||
disabled: disabledButton.apply(this), | ||
}, | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
/** | ||
* Copyright IBM Corp. 2018, 2019 | ||
* | ||
* This source code is licensed under the Apache-2.0 license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
/** | ||
* Template configuration generate functions for Button component | ||
mattrosno marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* @module button/generate | ||
*/ | ||
import cloneDeep from 'lodash/cloneDeep'; | ||
import merge from 'lodash/merge'; | ||
|
||
// TODO JSDoc | ||
export function generate(options) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'm confused by the open to new thinking though There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it depends on the two options stated here? This function here isn't meant to be a full template configuration object - although maybe it's close enough to act as that for Handlebars? The goal of the configuration object would be to give each component API just enough for it to render an accessible component. |
||
let opts = merge( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where else would the component default context be useful? I guess in the React world, defining const defaultButton = config.generate();
Button.defaultProps = {
iconDescription: 'Provide icon description if icon is used',
tabIndex: defaultButton.tabIndex,
type: defaultButton.type,
disabled: defaultButton.disabled,
small: defaultButton.size === 'small',
kind: defaultButton.variant,
}; Maybe that was with an older version of |
||
{ | ||
element: 'button', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the use-case for this variable please? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following Carbon React's lead for this POC. |
||
content: 'Button', | ||
mattrosno marked this conversation as resolved.
Show resolved
Hide resolved
|
||
attributes: {}, | ||
mattrosno marked this conversation as resolved.
Show resolved
Hide resolved
|
||
disabled: false, | ||
mattrosno marked this conversation as resolved.
Show resolved
Hide resolved
|
||
type: 'button', | ||
mattrosno marked this conversation as resolved.
Show resolved
Hide resolved
|
||
tabIndex: 0, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why the redundant tabIndex? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Carbon React sets |
||
variant: 'primary', | ||
}, | ||
options | ||
); | ||
|
||
const selectors = cloneDeep(this.selectors); | ||
|
||
let config = { | ||
root: {}, | ||
}; | ||
|
||
Object.keys(selectors.default).forEach(element => { | ||
config = merge(config, { | ||
[element]: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would force Icon to always be present but it's actually an optional element. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I figured that'd be up to carbon-components-x to decide. If they know to put an icon in the template, then can use this from the component config. If not, ignore and not use it. "icon": {
"attributes": {
"aria-hidden": true
},
"classNames": "bx--btn__icon"
} I think it depends if the button config is responsible for its child icon config, or if carbon-components-x can call button config (knowing there's an icon) and icon config do its thing. Doing the icon component next will help here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the icon component will help this debate. This seems like code that belongs in |
||
attributes: {}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if the user wants to define custom attributes on an element? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! I'll fix. |
||
classNames: selectors.default[element], | ||
}, | ||
}); | ||
}); | ||
|
||
// Element | ||
|
||
config.root.element = opts.element; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a non-anchor use-case for this variable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just for the anchor. |
||
|
||
// Content | ||
|
||
config.root.content = opts.content; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the classes object and the outputted object don't align
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Does that help? Or is something still misaligned? |
||
|
||
// Classes | ||
|
||
config.root.classNames += ` ${selectors.variants[opts.variant].root}`; | ||
|
||
if (opts.size) { | ||
config.root.classNames += ` ${selectors.modifiers.sizes[opts.size].root}`; | ||
} | ||
|
||
// Attributes | ||
|
||
if (opts.variant === 'danger') { | ||
config.root.attributes['aria-label'] = 'danger'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when I was testing this with a screen reader it did not read the content of the button... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like I did this because of Carbon Vanilla. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll remove this aria-label to follow Carbon React's lead. |
||
} | ||
|
||
if (opts.element === 'button') { | ||
config.root.attributes.type = opts.type; | ||
|
||
if (opts.disabled) { | ||
config.root.attributes.disabled = ''; | ||
} | ||
} else if (opts.element === 'a') { | ||
config.root.attributes.href = opts.href || '#'; | ||
config.root.attributes.role = 'button'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a button and a link have two different functions why are they being mixed here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following Carbon React's lead. Disabled anchors looks to be a bug. Not in POC scope. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. gotta agree with Liz - the anchor should not be in this component. The functionality to create an I think our goal in pointing this out is that it really doesn't belong in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A follow-up to this comment, because all Carbon implementations aren't in lock-step, why this spec is coming to be, I expect every implementation to start off with failing tests to some degree. Do you think we need to increase the scope of this proof of concept so that every Carbon implementation passes the spec for the four components? I don't think that's necessary as long as everyone can see a clear path on how to achieve that. Should be minimal technical debt if we're not modifying every carbon-components-x to pass full spec tests for our 4 components. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should focus only on button for now, and yeah, we should increase scope to work with all four frameworks to determine that the code we create works in all four. We don't want to go deep into building stuff out for four components only to find out it doesn't work with one or more frameworks. Getting this all in place via |
||
} | ||
|
||
if (config.icon) { | ||
config.icon.attributes['aria-hidden'] = true; | ||
} | ||
|
||
return config; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
/** | ||
* Copyright IBM Corp. 2018, 2019 | ||
* | ||
* This source code is licensed under the Apache-2.0 license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
/** | ||
* Available CSS selectors for Button component | ||
* @module button/selectors | ||
*/ | ||
|
||
/** | ||
* Button component classes | ||
* @typedef {object} buttonClasses | ||
* @property {string} root - button element | ||
* @property {string} content - wrapper element around button content | ||
* @property {string} icon - button SVG | ||
*/ | ||
|
||
/** | ||
* Button component selectors | ||
* @typedef {object} buttonSelectors | ||
* @property {buttonClasses} default - default button classes | ||
* @property {object} variants - button variants | ||
* @property {buttonClasses} variants.primary - `primary` variant | ||
* @property {buttonClasses} variants.secondary - `secondary` variant | ||
* @property {buttonClasses} variants.tertiary - `tertiary` variant | ||
* @property {buttonClasses} variants.ghost - `ghost` variant | ||
* @property {buttonClasses} variants.danger - `danger` variant | ||
* @property {object} modifiers - style modifiers | ||
* @property {object} modifiers.sizes - size modifiers | ||
* @property {buttonClasses} modifiers.sizes.small - `small` modifier | ||
*/ | ||
const selectors = { | ||
default: { | ||
root: 'btn', | ||
content: 'btn__content', | ||
icon: 'btn__icon', | ||
}, | ||
variants: { | ||
primary: { | ||
root: 'btn--primary', | ||
}, | ||
secondary: { | ||
root: 'btn--secondary', | ||
}, | ||
tertiary: { | ||
root: 'btn--tertiary', | ||
}, | ||
ghost: { | ||
root: 'btn--ghost', | ||
}, | ||
danger: { | ||
root: 'btn--danger', | ||
}, | ||
}, | ||
modifiers: { | ||
sizes: { | ||
small: { | ||
root: 'btn--sm', | ||
}, | ||
}, | ||
}, | ||
}; | ||
|
||
export default selectors; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
/** | ||
* Copyright IBM Corp. 2018, 2019 | ||
* | ||
* This source code is licensed under the Apache-2.0 license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
/** | ||
* Button component | ||
* @module button | ||
*/ | ||
import { prefixSelectors } from '../../tools/component/selectors'; | ||
import demo from './config/demo'; | ||
import { generate } from './config/generate'; | ||
import selectors from './config/selectors'; | ||
|
||
/** | ||
* Button spec | ||
* @param {string} prefix - selector prefix | ||
* @type {globalTypedefs.componentConfig} | ||
*/ | ||
const buttonConfig = prefix => { | ||
const config = { | ||
selectors: prefixSelectors(selectors, prefix), | ||
}; | ||
|
||
return { | ||
label: 'Button', | ||
demo: demo.bind(config), | ||
generate: generate.bind(config), | ||
selectors: config.selectors, | ||
}; | ||
}; | ||
|
||
export default buttonConfig; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
// | ||
// Copyright IBM Corp. 2018, 2018 | ||
// | ||
// This source code is licensed under the Apache-2.0 license found in the | ||
// LICENSE file in the root directory of this source tree. | ||
// |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
# | ||
# Copyright IBM Corp. 2018, 2018 | ||
# | ||
# This source code is licensed under the Apache-2.0 license found in the | ||
# LICENSE file in the root directory of this source tree. | ||
# |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
// | ||
// Copyright IBM Corp. 2018, 2018 | ||
// | ||
// This source code is licensed under the Apache-2.0 license found in the | ||
// LICENSE file in the root directory of this source tree. | ||
// |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
// | ||
// Copyright IBM Corp. 2018, 2018 | ||
// | ||
// This source code is licensed under the Apache-2.0 license found in the | ||
// LICENSE file in the root directory of this source tree. | ||
// |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
/** | ||
* Copyright IBM Corp. 2018, 2019 | ||
* | ||
* This source code is licensed under the Apache-2.0 license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
/** | ||
* Copyright IBM Corp. 2018, 2019 | ||
* | ||
* This source code is licensed under the Apache-2.0 license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
/** | ||
* Copyright IBM Corp. 2018, 2019 | ||
* | ||
* This source code is licensed under the Apache-2.0 license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/** | ||
* Copyright IBM Corp. 2018, 2019 | ||
* | ||
* This source code is licensed under the Apache-2.0 license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
import buttonConfig from '../'; | ||
|
||
describe('Button', () => { | ||
it('Generates a button', () => { | ||
const button = buttonConfig('bx'); | ||
const myButton = button.generate({ | ||
disabled: true, | ||
size: 'small', | ||
variant: 'danger', | ||
}); | ||
|
||
console.log(JSON.stringify(button.label, null, 2)); | ||
|
||
console.log(JSON.stringify(button.demo(), null, 2)); | ||
|
||
console.log(JSON.stringify(myButton, null, 2)); | ||
|
||
console.log(JSON.stringify(button.selectors, null, 2)); | ||
|
||
expect(true).toEqual(true); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I don't find "anchor" to be a "modifier" as there is a lot that goes in to making a "button" a link. It is not on the same level as making a button "small".
That was something we worked out extensively to define the difference between the terms
variant
,modifier
,state
, andscenario
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming being an "anchor" means we set different
element
and an additionalaria
role... playing along with Carbon React for a second... how would you classify this?Can you share those definitions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This terminology we use in the selectors object, config definitions, etc. For demo description specifics, we also have a
test
category which is used to describe a demo that is created specifically to test different setups/bugs and does not define a feature of a component.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This info should be documented somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know... I thought I had it on our site somewhere but couldn't find it. 📝