Update contribution guidelines

[sukhrajghuman]

No description provided.

[azerella]

👍 Not sure if we should separate the code do/don't into separate contributing docs but damn good start.

[azerella]

Suggested change

	function CalculateSpecs(initialSize,endSize,speed){
	function CalculateAnimationSpecs(initialSize,endSize,speed){

[ghost]

This example (Use new lines) more complex than at first glance and provides more questions than guidance

[ghost]

1. Mix concatenation and template string is quiet confusing in understanding which style you prefer. You are using and not using the power of template string in same time. And why here template string ${ dark ? ` au-control-input--dark` : '' }, but here ${ status === 'valid' ? ' au-control-input--valid' : '' } - not.
2. Why this line au-control-input ${ className } not splitted in

'au-control-input'  +
` ${className}` + ...

And if staring to split - split all. And should clarify the limit of allowed length of one line string.

// Good
<div prop={oneProp} longPropName={sameLongPropName} className={`classList`} >

// Bad, to long

<div prop={oneProp} longPropName={sameLongPropName} someVeryLongPropNameforSomeReason={propValue} >

// Should use new lines

<div
 prop={oneProp}
 longPropName={sameLongPropName}
 someVeryLongPropNameforSomeReason={propValue} 
>

// Don't

<div
 prop={oneProp} longPropName={sameLongPropName}
 someVeryLongPropNameforSomeReason={propValue} 
>

3. In that example in think it's better to recommend not to calculate class name with many conditions in template. It's hard to read. Extract calculations and use array so you don't need to worry about space before class name and mix template stings and concatenation.

// to much calculations in template
<div className={
  `au-control-input ${ className }` +
  `${ dark ? ` au-control-input--dark` : '' }` +
  `${ alt ? ` au-control-input--alt` : '' }` +
  `${ small ? ` au-control-input--small` : '' }` +
  `${ block ? ` au-control-input--block` : '' }` +
  `${ status === 'valid' ? ' au-control-input--valid' : '' }` +
  `${ status === 'invalid' ? ' au-control-input--invalid' : '' }`
} 
  otherProp={propValue}
>

// extract calculations

const classes = [
		'au-control-input',
		className,
		dark ? 'au-control-input--dark' : '',
		alt ? 'au-control-input--alt' : '',
		small ? 'au-control-input--small' : '',
		block ? 'au-control-input--block' : '',
		status === 'valid' ? 'au-control-input--valid' : '',
		status === 'invalid' ? 'au-control-input--invalid' : ''
	]
		.filter( cls => cls !== '' )
		.join(' ');

<div className={classes} otherProp={propValue} >

[sukhrajghuman]

Hey @toxamiz,

Thanks for the feedback! I think a lot of this could be fixed if we can resolve #429. I've seen you've opened up a PR so thanks for that!

1. Yeah this is probably just a bit of inconsistency. The code base is a couple of years old. We aren't perfect by a long stretch 😅. This is why we created Add a sass and javascript linter #429 and are updating the contributing guidelines. This is for everyone 🙂

2. I agree 100%.

3. Hmmm I think the way it's done now is cleaner and easier to read. I guess that's subjective. @adamzerella thoughts?

[azerella]

I agree with @sukhrajghuman here, option 1 reads nicer to me and doesn't require calling two additional functions

[ghost]

Hmmm I think the way it's done now is cleaner and easier to read. I guess that's subjective.

Yes, it is subjective :)

I prefer to keep template clear. I want to see the result of calculation, not how they been calculated. Except some simple operations like comparison and single ternary operator. If i want to know calculation process i would go to related method/function.

something like this:

// Option 1
// So what do we have
<div className={ // classname caclulation
  `au-control-input ${ className }` + // ok
  `${ dark ? ` au-control-input--dark` : '' }` + // ok 
  `${ alt ? ` au-control-input--alt` : '' }` + // ok
  `${ small ? ` au-control-input--small` : '' }` + // ok
  `${ block ? ` au-control-input--block` : '' }` + // ok
  `${ status === 'valid' ? ' au-control-input--valid' : '' }` + // ok
  `${ status === 'invalid' ? ' au-control-input--invalid' : '' }` // ok
} 
  someVeryImportantProp={propValue} // aha, that what i need
>

// Option 2
// Some classname calculation. Not interested, skip
const classes = [
		'au-control-input',
		className,
		dark ? 'au-control-input--dark' : '',
		alt ? 'au-control-input--alt' : '',
		small ? 'au-control-input--small' : '',
		block ? 'au-control-input--block' : '',
		status === 'valid' ? 'au-control-input--valid' : '',
		status === 'invalid' ? 'au-control-input--invalid' : ''
	]
		.filter( cls => cls !== '' )
		.join(' ');

// 
<div className={classes} somsomeVeryImportantProp={propValue} >  // aha, that what i need

[azerella]

Yes, it is subjective :)

It is, but we can look for evidence among industry when we are trying to come to a census. For example, AirBnB prefer template literals in their style guide:

Why? Template strings give you a readable, concise syntax with proper newlines and string interpolation features.

Source: http://airbnb.io/javascript/

Ultimately it comes down to the decision of the core team, we have developed our coding style over the span of years and don't see a compelling reason to change it.

[ghost]

@adamzerella I was talking about extracting logic from template, not about using concatenation via array.join instead of template strings.

const classes = `au-control-input ${ className } { dark ? 'au-conlatrol-input--dark' : '' } ${ alt ? 'au-control-input--alt' : '' } ${ small ? 'au-control-input--small' : '' } ${ block ? 'au-control-input--block' : '' } ${ status === 'valid' ? 'au-control-input--valid' : '' } ${ status === 'invalid' ? 'au-control-input--invalid' : '' }`

// or better

const classes = `au-control-input` +
 (className ? ` ${className}` : ``) +
 (dark ? ` au-conlatrol-input--dark` : `` ) + 
 (small ? ` au-control-input--small` : ``)  +
 (alt ? ` au-control-input--alt` : `` ) +
 (block ? ` au-control-input--block` : ``) + 
 (status === 'valid' ? ` au-control-input--valid` : ``)
 (status === 'invalid' ? ` au-control-input--invalid` : `` )
 

<div className={classes} otherProp={otherProp} >

[azerella]

@toxamiz Can you please provide some references of other JavaScript code-bases doing this?

@sukhrajghuman and I aren't convinced that this should be in our style guide

[ghost]

Can you please provide some references of other JavaScript code-bases doing this?

Extracting logic from template?

1. Airbnb react guide (not many examples) https://github.com/airbnb/javascript/tree/master/react#props
2. Airbnb some react project - https://github.com/airbnb/react-sketchapp/blob/master/src/components/Image.js
3. Google material-ui - https://github.com/mui-org/material-ui/blob/next/packages/material-ui/src/Grid/Grid.js

and I aren't convinced that this should be in our style guide

May be you right. It is more about reviewing particular case like "Your template is hard to read, please move that logic to separate method/variable/etc"

[azerella]

Closing this in favour of #708. We need more time to look at our coding standard. In the meantime #700 is getting close to set the standard for us.