[WIP] Button

[mattrosno]

yarn install && yarn test

Check out src/components/button/tests/unit.js for a quick and simple demonstration of how a carbon-components-x implementation would instantiate a button (with a styling prefix) along with generating a button config, which then the carbon-component-x would use to render the button according its library/framework best practices.

Comparing to previous efforts, this supports class name prefixes. The button configuration object includes:

• label - "Button"
• demo - Object of demo objects so we can reference demos by name in testing efforts. These objects are Fractal-friendly. If carbon-components-x uses Fractal for development or documentation purposes, src/tools/component/demo has the flattenDemosObject function that you can call once you have your component config prior to rendering in Fractal.
• generate - Just one for button, but expected to be many for more complicated components.
• selectors - If you need it, the full selectors object with prefixed class names.

To Do

• Config
• Requirements
• Test runner
• Tests
• JSDoc

[elizabethsjudd]

Thanks for taking this on Matt! I wasn't able to get through everything tonight but wanted to get back to you with what I had. Overall, I think a needs to be removed, it's semantically a different component and should be treated as such. I could see the justification of making an "easy" way to do this but in that case the button should be calling the link generator instead of mixing the two components in one generate. The generate function is also lacking a lot of flexibility for our end users.

[elizabethsjudd]

I feel like using an a tag isn't really valid for a button. I understand that often links look like buttons but semantically it's still a link and they have different requirements that affect their HTML structure. I'd recommend having links outside the button component but you're allowed to style a link as a button (see the button demo in link.config.js of PAL

[mattrosno]

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 v10 React component visuals and behavior will be used in the proof of concept as standardizing component implementations is not in scope

Carbon React button component can render as either button or a, so this proof of concept does as well, so we can focus on the bigger issues at hand.

[scottnath]

We obviously don't want to change what React does at this point...but if our goal is to focus on button, then that should keep the focus on what will eventually be the <button HTML element - of which a link (<a) would be out-of-scope as it's a totally different element with different a11y requirements. The React Button could still be tested with this code, we just wouldn't test against the anchor section - as there is not a compliant scenario where a <button would be an anchor element.

Testing only the button aspects of the React component would be in scope as we should expect any of the frameworks to have additional code and functionality in any given component - and our goal with the spec is to say "do you create these specific UI pieces" rather than "do you only create these specific UI pieces"

[mattrosno]

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.

[scottnath]

So, we're not trying to create a perfect button by pushing back on anchor, its more about keeping button within it's basic scope of being a button as opposed to the React implementation's expanded use cases for a React component called Button. I do see that #5 includes Carbon v10 React component visuals and behavior will be used in the proof of concept as standardizing component implementations is not in scope so I can see where you're coming from, but I believe that misses out on a very important aspect of working to sync up across frameworks: User Expectations.

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:

1. how can we write tests to confirm all frameworks are in sync
2. how can we come up with a common way to write requirements so our users can understand why choices were made about a component and our framework developers can understand what outcomes are expected
3. how can components share a common api?

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 requirements functionality. But in this case, there is a clear issue with this specific demo. A demo that would be very easy to leave it out of this POC. We can tackle the need to support variable elements in some of the more complicated components, but the point of starting with button is indeed it's lack of complication.

[elizabethsjudd]

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

[mattrosno]

I did this to bind this.selectors used in the generate functions. this.selectors gets set when you instantiate the config with a prefix.

I'm open to other ways to handle prefixing to avoid binding and applying functions. Suggestions?

[elizabethsjudd]

why the redundant tabIndex?

[mattrosno]

Carbon React sets tabIndex, so this proof of concept does as well.

[elizabethsjudd]

when I was testing this with a screen reader it did not read the content of the button...

[mattrosno]

Looks like I did this because of Carbon Vanilla.

[mattrosno]

I'll remove this aria-label to follow Carbon React's lead.

[elizabethsjudd]

a button and a link have two different functions why are they being mixed here?
What about disabled anchors? (I feel like links should NOT be part of this component)

[mattrosno]

Following Carbon React's lead. Disabled anchors looks to be a bug. Not in POC scope.

http://react-experimental.carbondesignsystem.com/?selectedKind=Buttons&selectedStory=Default&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybooks%2Fstorybook-addon-knobs&knob-Button%20kind%20%28kind%29=primary&knob-Disabled%20%28disabled%29=true&knob-Small%20%28small%29=&knob-Icon%20%28icon%29=none

[scottnath]

gotta agree with Liz - the anchor should not be in this component. The functionality to create an anchor within button is specific to the React component. It looks like anchor was originally added to this component for styling purposes, which, if that's the case, the styling of an anchor to look like a button should happen within the anchor tag's component, applying button styles with classes.

I think our goal in pointing this out is that it really doesn't belong in the spec for a button, and by keeping it in here from the start, we're adding technical debt from the start. This would not be a use of button which we would be able to support due to compliance and a11y issues, so when running the spec against the handlebars button, we'd start off as failing the spec.

[mattrosno]

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.

[scottnath]

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.
The goal is to make sure all four frameworks can at least get the tests running, but not necessarily that they all need to pass the tests.

Getting this all in place via button only will allow us to iterate on a known-to-be-working-cross-framework base when building out the other 3 components - which will limit how much tech debt we accrue.

[elizabethsjudd]

the classes object and the outputted object don't align

{
   root: ...
   content: ...
   icon: ...
}

{
   root: {
    classNames: ...
    content: ...
    attributes: ...
  },
  content: {
    classNames: ....
  },
  icon: {
     classNames: ...
     attributes: ...
  }
}

[mattrosno]

content is overloaded in this example, because of the bx--btn__content element? I know I broke my rule here of following Carbon React's lead since they don't have that inner span. I called that inner span bx--btn__content to show how content can be a node's name in selectors object, and still be used alongside element, attributes, className for text.

Does that help? Or is something still misaligned?

[elizabethsjudd]

This would force Icon to always be present but it's actually an optional element.

[mattrosno]

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?

[scottnath]

I think the icon component will help this debate. This seems like code that belongs in Icon

[elizabethsjudd]

what if the user wants to define custom attributes on an element?

[mattrosno]

Good catch! I'll fix.

[elizabethsjudd]

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, and scenario.

[mattrosno]

Assuming being an "anchor" means we set different element and an additional aria role... playing along with Carbon React for a second... how would you classify this?

Can you share those definitions?

[elizabethsjudd]

• variant - larger styling/functionality differences. You can't mix variants on the same instance. Examples include:
  • primary, secondary, etc for buttons
  • required action, transactional, passive modals
• modifier - small change that can be applied to any/multiple variants. However modifiers of the same "category" can not be applied to the same instance. Examples include:
  • Size: such as trying to add compact/comfy (small/large) classes to the same instance
  • row differentiators for tables (alternating colors vs borders)
  • direction of a tooltip in relation to it's trigger
• state - change caused by the user interacting with an element/component
• scenario - changes the HTML structure/content model (e.g. button with an icon, data table with a toolbar, tooltip with a heading, etc)

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.

[scottnath]

This info should be documented somewhere.

[elizabethsjudd]

I know... I thought I had it on our site somewhere but couldn't find it. 📝

[scottnath]

This is very cool. My two main concerns:

• does generate belong here
• can we please remove anchor code

[scottnath]

We obviously don't want to change what React does at this point...but if our goal is to focus on button, then that should keep the focus on what will eventually be the <button HTML element - of which a link (<a) would be out-of-scope as it's a totally different element with different a11y requirements. The React Button could still be tested with this code, we just wouldn't test against the anchor section - as there is not a compliant scenario where a <button would be an anchor element.

Testing only the button aspects of the React component would be in scope as we should expect any of the frameworks to have additional code and functionality in any given component - and our goal with the spec is to say "do you create these specific UI pieces" rather than "do you only create these specific UI pieces"

[scottnath]

what is the use-case for this variable please?

[mattrosno]

Following Carbon React's lead for this POC.

[scottnath]

• it looks like opts is the default context a button component would receive - it would be handy to have this be an object outside of generate so it could be exported and used in other places.
• we switched to using mergeWith due to merging issues we ran into with arrays. merge will munge arrays together with unintended consequences - so merge([1, null, 2, 3], [1, 5, 6]) becomes [1, 5, 6, 3] or similar weirdness

[mattrosno]

Where else would the component default context be useful? I guess in the React world, defining defaultProps, so you could get those without generating a default component. But then again, what's wrong with calling an empty generate function to get the default component config? E.g.

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 merge? Looks like no difference if not using a customizer. https://codepen.io/mattrosno/pen/MLbPwM?editors=0011

[scottnath]

I think the icon component will help this debate. This seems like code that belongs in Icon

[scottnath]

• putting the context which is used by the generate function within the generate.apply call means that that object is not available for use by the frameworks, all they would receive is the post-generate object. We put this in a separate preGenerate variable, which allows both use of the smaller config object directly...as well as giving us access to the preGenerated object in documentation
• why is this within an array?

[mattrosno]

1. Have you found use for that pre-generate object outside of documentation? I'll update.
2. Second parameter of function.apply() is of type array.

[scottnath]

I think I'm confused by the generate function being in the spec. My thoughts were that an individual repo would take care of this level of integration. i.e. the simpler api {variant: 'primary'} would be what is shared by c-c-X, and each repo would be in charge of converting that to the proper template configuration object for their implementation.

open to new thinking though

[mattrosno]

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.

[scottnath]

Hey Matt,

Added longer comments blow on the two major points.

Other than those two comments, I think we should revisit whether the generate function belongs in the spec repo for now. I think simpler is better for what spec export for demo config. The end result of creating a template config should be up to the framework. I think after working with all four frameworks we might be able to come up with a more expanded demo config object, but we should first figure out if a simple {variant: 'danger', disabled: true} could be used by all four frameworks to get the same result. Doing it this way gives each framework more flexibility in how to implement the selectors object, among other items

[scottnath]

So, we're not trying to create a perfect button by pushing back on anchor, its more about keeping button within it's basic scope of being a button as opposed to the React implementation's expanded use cases for a React component called Button. I do see that #5 includes Carbon v10 React component visuals and behavior will be used in the proof of concept as standardizing component implementations is not in scope so I can see where you're coming from, but I believe that misses out on a very important aspect of working to sync up across frameworks: User Expectations.

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:

1. how can we write tests to confirm all frameworks are in sync
2. how can we come up with a common way to write requirements so our users can understand why choices were made about a component and our framework developers can understand what outcomes are expected
3. how can components share a common api?

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 requirements functionality. But in this case, there is a clear issue with this specific demo. A demo that would be very easy to leave it out of this POC. We can tackle the need to support variable elements in some of the more complicated components, but the point of starting with button is indeed it's lack of complication.

[scottnath]

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.
The goal is to make sure all four frameworks can at least get the tests running, but not necessarily that they all need to pass the tests.

Getting this all in place via button only will allow us to iterate on a known-to-be-working-cross-framework base when building out the other 3 components - which will limit how much tech debt we accrue.