This issue has been spotted by @msinga for a feature we are working on:

"Safari browser- Errors are being truncated"

I have fixed it by applying this css directly in our application for the component :
.ui-tax-number-with-suffix__faux-textbox { .common-input__message-wrapper { word-break: break-word; } }

Although the word-break property should be set on every message-wrapper in carbon.
This is only affecting Safari as I believe by default the other browsers apply break-word.

As part of the carbon we could offer templates of common patterns such as a grid/table with a dialog. This template could also include the base bones flux architecture to get the user started

Currently we mimic the styling for validation messages, but we should also recreate the behaviour (dynamic positioning etc)

As we are breaking single, large API requests into smaller multiple requests, we need to be able to call them easily. This has been achieved through the use of promises.

We could simplify this by providing an API in Carbon to handle this, for example:

let requests = [{
  url: '/foo',
  data: this.myFooData,
  action: 'GET'
}, {
  url: '/bar',
  data: this.myBarData,
  action: 'GET'
}];

new BulkRequest(requests, this.onSuccess, this.onFail);

Behind the scenes this will use promises to fulfil the request. Once our server is able to handle Bulk requests we could then alter the internals of this class to perform the requests based on the new API meaning the app benefits from the change without any alteration of the service code.

Grids currently display data, however in some situations we may want to render other components such as buttons.

How should we handle this behaviour?

A user is unable to navigate through the list of items in a UI dropdown using the up or down keys on a keyboard.

To recreate:

• Click into the Province dropdown
• Try and use the Up and Down keys

This should work before and after searching :)

texts on buttons needs to support I18N
https://github.com/Sage/carbon/blob/master/src/components/form/form.js#L323
https://github.com/Sage/carbon/blob/master/src/components/form/form.js#L285

Basically all common modules like common-input would be prefixable with a BEM fragment.

<CommonElement classPrefix='my-specific-module' />

Would result in the rendered HTML:

<div class='common-element my-specific-module__common-element' />

What would the pros, cons and implications be of this?

I can see

1. The scss from My Specific Module would only have modular BEM classes in it and there'd be no nesting
2. A clear definition between Common Elements and Specific Modules would have to be defined in the source order

we would probably need to include a clearfix in many parts of our Carbon application.
Maybe we can define it as a globally available mixin (like here: http://compass-style.org/reference/compass/utilities/general/clearfix/)?

Need to rename all the component index.js files into their corresponding component names, and make the necessary changes in the package.json. Need to rename the associated scss files in the same manner. The structure will be as follows:

components/
    — dropdown-suggest/
        — dropdown-suggest.js
        — dropdown-suggest.scss
        — package.json
        — __spec__.js

I thought I'd put this out there for discussion amongst those interested.

.block {  }
.block__element {  }
.block--modified {  }
.block__element--modified {  }
.block--modified .block__element {  }

.block {
  &--modified { 
    .block__element {  }
  }
  &__element {
    &--modified {  }
  }
}

These two methods both produce the same compiled css.

I like the second method because it makes the source less verbose and makes the structure of the relationships between classes more clear. It's almost like an object with properties and methods (the properties being the elements and the modifiers, which typically are used for responding to events in the UI, being the methods).

I've used this method extensively in my own work.

Thoughts?

A Flux store may have several different pieces of data. Using a contact as an example we might have:

• Data for the contact (pulled from the server if editing).
• Flags for the state of the view (dialogs that are open, messages that are visible).
• Data for options such as dropdowns (probably pulled from the server).

It would be good to standardise how we structure the store for this data:

For the above I was thinking something like:

{
  "contact": {
    // data from the server or that will be posted to the server for the contact
  },
  "flag_for_some_state": true, // flags for the state at the top level, but separate from server data
  "contact_types": [
    // options for contact type dropdown
  ] 
}

Any thoughts?

Fonts in stylesheets are currently hardcoded to use /assets as the path, however developers should be able to configure this - perhaps with a Sass variable in the config file.

Should we introduce a type system into the carbon library. There have been a couple of cases where we are expecting integers but get strings and vice versa. A type system should stop these errors from occurring and compile time(?) or at least be obvious to developers at run time.

Here a couple of options:
http://flowtype.org/
http://www.typescriptlang.org/

I believe that both systems can also be introduced slowly into a project. So we could only add typescript/flow into a couple of files and see how we like it

If the carbon dialog is placed in an DOM element that is positioned (e.g. somewhere in the <div id="ui-main"> element in sage_one_advanced), the dialog is positioned depending on that element not on the whole window.

the code for centering the dialog in Carbon:

  centerDialog = () => {
    let height = this._dialog.offsetHeight / 2,
        width = this._dialog.offsetWidth / 2,
        midPointY = window.innerHeight / 2 + window.pageYOffset,
        midPointX = window.innerWidth / 2 + window.pageXOffset;

    midPointY = midPointY - height;
    midPointX = midPointX - width;

    if (midPointY < 20) {
      midPointY = 20;
    } else if (Bowser.ios) {
      midPointY -= window.pageYOffset;
    }

    if (midPointX < 20) {
      midPointX = 20;
    }

    this._dialog.style.top = midPointY + "px";
    this._dialog.style.left = midPointX + "px";
  }
// https://github.com/Sage/carbon/blob/v0.21.0/src/components/dialog/dialog.js#L135-L156

The code for centering the dialog in Sage One legacy UI:

  center: ($element, zeroScrollTop) ->
    elementHeight = $element.outerHeight()
    elementWidth = $element.outerWidth()
    windowHeight = window.height()
    windowWidth = window.width()
    scrollTop = window.scrollTop()
    scrollLeft = window.scrollLeft()
    calculatedTop = ((windowHeight - elementHeight) / 2) + scrollTop

    if (zeroScrollTop)
      # Still want window.scrollTop to be calculated initially, even if we remove it later.
      calculatedTop = calculatedTop - scrollTop

    if (calculatedTop < 50)
      calculatedTop = 50

    $element.offset
      top: calculatedTop,
      left: ((windowWidth - elementWidth) / 2) + scrollLeft

I tried to calculate midpoints like this:

midPointX = this.refs.dialog.offsetParent.offsetWidth / 2
midPointY = this.refs.dialog.offsetParent.offsetHeight / 2

which works well for my dialog, but makes the dialog to be rendered a bit too high.

Probably we should use a similar code to the one used in legacy Sage One, because it was already tested in a lot of contexts.

Checkbox and RadioButton components are very similar. They could probably extend from one another to DRY the code up some.

Carbon requires its own dirty forms mechanism to ensure users don't accidentally lose data with backspace or links.

Currently, we have a hack to remove the last path from the url. We need to update this to more intelligently retrace the user's path.
In Rails we do something like history.back() for cancel urls unless a developer specifies a specific url for the cancel button.

From the introduction of the Table widget and the Flash widget we have introduced the small ClassNames library which can help tidy up our code. In many places we have logic to check

get mainClasses() {
  let className = 'ui-bla'

  if (this.props.className) {
    className += 'ui-bla'
  }
}

Instead we could use

get mainClasses() {
  return classNames('ui-bla', this.props.className)
}

In our current setup we precompile (through Babel) our JS code from the /src directory into the /lib directory.

We do this to speed up compilation for developers when using anything from Carbon, as the gulp task does not have to run the Carbon code through the Babel compiler as it is already compiled for them.

However, it does have some negative effects.

• It effectively doubles the source code in the Git repo.
• It is confusing for developers why there is a lib and src directory, and where they should make any changes.

As an alternative, instead of commiting the lib directory into Git, it can be generated whenever a developer installs Carbon through npm. For example when running npm install carbon. This means it is available to a developer when they need it, and isn't cluttering the Git repo. For this to work, the developer will require having Babel installed globally on their machine.

What do other repos do?

Another reason we offer the precompiled code is that some developers may not want to use Babel - so we can give it to them already compiled. On top of this, they may not want to use any Node compiler at all - in which case we could even offer a fully compiled JS file which they could drop in browser without any compilation task

An example of a repo which does this is this component.

• They have a src
• They have a lib
• They have a dist

This is beneficial for open source projects, which Carbon is not yet, so maybe we do not have to cater for this at this point in time, and can get away with precompilation on install instead.

Looking for any thoughts/suggestions?

A styleguide should be added to the carbon library so that contributors are aware of how to structure the code correctly. This style guide can probably be borrowed and edited from another source.

We should then review/update the code base to ensure that it follows the styleguide we introduce

When wrapping rendered components in Input(InputValidations(myComponent)), the current API expects the component to live inside a Form. If the component is not rendered in a Form, the view will break.
We should add logic for each component rendered with InputValidations or with a Form Context to check whether it lives in a form.

For an example see credit terms in the contact creation dialog.

Should not use component.props.name to store reference to the input in the form, as this relies on the developer to always have unique names. Form validation should also skip inputs that are disabled.

We're looking at the data currently being used with the work done in React/Carbon, and we're trying to figure out a standard for naming conventions etc.

For JavaScript variables it is common to use camel case, for example:

var myVariable = "foo";

function sampleFunction() {};

We also work with JSON data form API endpoints. As we are using Rails, ours returns data in snake case, for example:

"data": {
  "my_variable": "foo",
  "sample_text": "bar"
}

From what I have seen, this seems to be fairly common across APIs.

When we retrieve data into React, we put it in a store using Immutable.js. So far, we have mapped this in the store converting it from snake case into camel case - such as here:

this.data=this.data.set('[addresses_attributes][0][country_id]', data.get('default_country_id'));

Then when we post the data back to the store we have to convert it back into a format the server wants (back into snake case).

Personally, I think we should keep using camel case for JavaScript variables, but use snake case for interacting with JSON data. For example:

// example of retrieving from the store:
myStore.get("sample_variable");
// example of setting to the store:
myStore.set("sample_variable", "new value");

This way, we don't need to map every variable from the API request to convert it into camel case - and when we post to the server our data is already in a format the server will respond to.

Does anyone else have any opinions or ideas about this?

Having said this however, posting back to the server may sometimes need some manual mapping of data - so this does not mitigate it completely, but should reduce it.

Decimal component should only allow users to enter 2 decimal places.

However, developers should be able to set custom precision on decimal components if they want users to be able to enter more than to 2 decimal places.

Changes made to the store data in one spec file are visible in other spec files.

It would be good to have a resetStore method available in each store - maybe that method could be run automatically beforeEach spec?

CSS properties naturally lend themselves to logical groupings and a logical grouping is better than an arbitrary order. Logical groupings can also be sensibly divided, benefiting from the principle of proximity, rather than using some other arbitrary function like every 5:

width: 50px; // x then y
height: 50px;

background-color: #f07;
opacity: 0.5;

border: none; // box edge properties grouped
margin: 10px 20px;
padding: 15px;

position: absolute;
top: 0; // clockwise from top, like border or margin
right: 0;
bottom: 0;
left: 0;

is better than:

background-color: #f07;
border: none;
bottom: 0;
height: 50px;
left: 0;
margin: 10px 20px;
opacity: 0.5;
padding: 15px;
position: absolute;
right: 0;
top: 0;
width: 50px;

Groupings can be consistent across selectors

This is not recognised as 'best practice' but best practice is there to be challenged - if we didn't we'd still be using IDs in css and nesting heavily in Sass.

What do you think?

Dropdown issues

Up and Down keys

Currently moving through dropdown results with the arrow keys does not change the position of the scroll.

In a couple of components we are using the toJS method on our immutable data structures which creates a full copy of the data. This is an expensive operation and the code should be changed to fully utilise the Immutable data structure

Currently, for dialog, we pass a prop of open to the component to determine if it is open or not.

It does not render its contents if this prop is set to false, but it still renders a few divs on the page.

Would a better performant API for this instead remove the need for the open prop, and instead rely on the developer to either render the component or not render the component?

Is there much benefit to this? Could devs just extend and override what they need to?

JavaScript is pretty poor at working with integers/numbers and floating point integers. The DOM is also not very consistent with returning values of the correct type, for example we retrieve an integer from the server, assign it to a HTML input and then when we retrieve it from the input it is now a string.

For this reason, should we avoid using anything other than Strings? (similarly, we should be using BigNumber.js for calculations).

We could enforce the use of strings by updating our ImmutableHelper to convert any numbers parsed to be strings - so developers don't have to worry about it.

In several components we would like to define a prop of type, for such values as warning, error etc

type however is a valid HTML property for some elements so we cannot use it reliably. We need to standardise we we should use for this.

Open for suggestions?

Maybe as or status?

https://facebook.github.io/react/docs/more-about-refs.html#the-ref-callback-attribute

Should we be doing this more consistently?

The example page is getting very busy with components. Should we look to add more example pages that separate the different types of components which can eventually be ported to some other host (github pages when open-source).

Trying to install Carbon with Babel 6 currently throws an error.

Update out the carbon app to use const

Exert from https://strongloop.com/strongblog/es6-variable-declarations/

Constant Reference, Not Value
The other new keyword for variable declaration in ES6 is const, but it is often misinterpreted as being a “constant value”. Instead, in ES6 a const represents a constant reference to a value (the same is true in most languages, in fact). In other words, the pointer that the variable name is using cannot change in memory, but the thing the variable points to might change.

Here’s a simple example. In the code below we create a new variable with a constant reference to an Array. We can then add values onto the array and since this does not change the reference, everything works:

const names = [];
names.push( "Jordan" );
console.log( names );

However, if we try to change the variable reference to a new array – even to one with the same contents – we will get a SyntaxError (“Assignment to constant variable”):

const names = [];
names = [];  // Error!

Of course, if you have a const that points to a primitive such as a string or number, then there really isn’t anything to change about that value. All methods on String and Number return new values (objects).

The last note about using const is that it follows the same new scoping rules as let! This means that between let and const we should be able to complete replace var in our code. In fact, there are many folks supporting the idea of only “allowing” var to be used in legacy code that hasn’t been touched. When a developer jumps into a file to update some code, they could (and possibly should) be updating all var statements to let or const as appropriate, with proper scoping.

Length warnings are firing from the input decorator when doing this comparison:

https://github.com/Sage/carbon/blob/master/src/utils/decorators/input/input.js#L65-L66

It appears to be because the mapping creates a new Immutable object maybe?

We have been looking into performance in React components more recently, and found some things what we should consider best practise.

If you have not already, you should have a read of the official documentation on advanced performance from Facebook https://facebook.github.io/react/docs/advanced-performance.html

Currently, on every single event (for example a keypress), every single component performs an "update". This sounds unperformant, but React's performance actually comes in with their Virtual DOM. When a component "updates" it makes a diff with what is to be rendered and what is currently in the Virtual DOM - it can then determine the minimum amount of changes it needs to make to the real DOM. Generally, there will be no changes so all is good!

With modern browsers we will not notice any performance issues. However, for older browsers we can make the performance even better with the use of shouldComponentUpdate. Every component can use this method to perform a manual check for whether it should even try and "update".

In Carbon, we currently do this on all inputs.

All we do here is do a deep comparison between the next props and state against the current props and state. If they have not changed, then we skip the "update" for this component.

We recommend developers utilise this within their own components too.

For example we might have a complex component broken up into 5 smaller sub-components. If none of these implemented shouldComponentUpdate then there would be a lot of needless updating going on. However if they each implemented their own shouldComponentUpdate then a change in one sub-component would not bother updating the other 4 sub-components.

We provide a helper function as of Carbon v0.3.3 which should make this easier. For example:

import React from 'react';
import shouldComponentUpdate from 'carbon/lib/utils/helpers/should-component-update';

class MyComponent extends React.component {
  /**
   * Reacts lifecycle method for checking for updates
   */
  shouldComponentUpdate(nextProps, nextState) {
    // run our custom update checker
    return shouldComponentUpdate(this, nextProps, nextState);
  }
}

export default MyComponent;

This will only work if you are only passing the required props down to components. We should not be passing the entire store between components.

This also means that sub-components will be easier to understand as we can clearly see what properties they depend upon.

Should incorporate methods like:

typedCharacter = String.fromCharCode(ev.which)

# Allow entering of backspace or unrecognized (e.g. arrow) keys.
code = typedCharacter.charCodeAt(0)

return true if code is 0 or code is 8

input = $(ev.target)

# Need to use jQuery caret() plugin because it solves
# browser differences in handling cursor/selection.
valuePrefix = input.val().substring(0, input.caret().start)
valueSuffix = input.val().substring(input.caret().start + input.caret().text.length, input.val().length)

value = valuePrefix + typedCharacter + valueSuffix

scale = parseInt(input.attr("data-scale"))

isValidDecimal(value, scale)

and here:

isValidDecimal = (value, scale) ->
  del = I18n.t("number.format.delimiter")
  sep = I18n.t("number.format.separator")
  regex = new RegExp('^[+-]?[0-9]{0,3}(?:\\' + del + '?\\d?)*[\\' + sep + ']?\\d{0,' + scale + '}$')
  result = regex.test(value)
  result

to properly format decimal

We currently use class properties to save manually binding functioning to the class in its constructor. However, we are finding issues overriding functions defined this way. Maybe our base components should all manually set any functions on themselves, allowing for more consistency and more flexibility.

In most of the components within Carbon we break up the render method into smaller functions which contain logic and return JSX. An example can be found here.

Would it be more performant if the function was changed to be a separate react class that lived in the same file. Taking the input grid as an example if the header was separated into its own class react would be able to identify if the header needs to be re-rendered before it does any extra work.

We could also add a shouldComponentUpdate to the smaller class to make the component more performant. An example of splitting a component up into smaller react classes can be found here.

React recommends using checked for checkboxes (https://facebook.github.io/react/docs/forms.html#interactive-props) - do we still need to set value?

When Decimal widget is used with label:
<Decimal label='Test' name='test' value={ 3.00 } />, the error icon and message are positioned outside the input box.

While rendering a link inside

        <Button  href={ 'tax_returns/new?taxType=' + taxType }
                          as='primary'
                          >
           'My Button'
        </Button>

The default colour is not correct. It should be white text with blue back ground.

Some I18n strings may contain HTML.

In React, the only way to successfully render the HTML within the string is to use dangerouslySetInnerHTML - https://facebook.github.io/react/tips/dangerously-set-inner-html.html

This is labelled for the reason that it could be dangerous, if an attacker is able to manipulate the I18n translation then they could potential render dangerous HTML.

To get around this we could use something like DOMPurify - https://github.com/cure53/DOMPurify

To make it even easier for developers, so they don't have to worry about it, we could create a I18n helper component which could look like this:

<Message>
  <I18n key="my.key" html={ true } />
</Message>

Under the hood it might look like this

get parseTranslation() {
  return DOMPurify(this.translation);
}

get translation() {
  return I18n.t(this.props.key);
}

get markup() {
  if (this.props.html) {
    return <span dangerouslySetInnerHTML={ this.parseTranslation } />;
  } else {
    return <span>{ this.translation }</span>;
  }
}

render() {
  return this.markup;
}

Currently grids read and render data based on key value pairs. However, in some cases data maybe provided in the format of a dropdown with a nested object containing and ID.

How should we handle this behaviour?

We should include the Sage Icon Font.

This is a discussion about having inline styles within our react components.

Ive seen a few discussions around having inline styles in your component. These inline styles would be purely structural. For example the position of the list for the Dropdown component. Colours would remain external the component.

I believe people are making this shift to inline styles because they want their components to be easily consumed by others. Having all javascript, html (JSX) and structural css within the one file would mean users of the common library get everything they need to render the component correctly without worrying about how to include our styles within their build.

This would add more complexity to the components and we would have to allow for the styles to be overwritten via a prop to the component.

https://www.youtube.com/watch?v=ERB1TJBn32c