Skip to content
This repository was archived by the owner on Aug 26, 2021. It is now read-only.

Update contribution guidelines #699

Closed
Parent: Merge develop to master
wants to merge 9 commits into from
Closed

Update contribution guidelines #699

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3868293
Update CONTRIBUTING.md
sukhrajghuman Mar 24, 2019
2f53ae4
Update CONTRIBUTING.md
sukhrajghuman Mar 24, 2019
0bb6f07
Update CONTRIBUTING.md
sukhrajghuman Mar 24, 2019
bd4843b
Update CONTRIBUTING.md
sukhrajghuman Mar 24, 2019
f879d72
Update CONTRIBUTING.md
sukhrajghuman Mar 24, 2019
d3092e7
Update CONTRIBUTING.md
sukhrajghuman Mar 24, 2019
621b6ae
Update CONTRIBUTING.md
sukhrajghuman Mar 24, 2019
ac0635b
Update CONTRIBUTING.md
Mar 26, 2019
b8090fb
Update CONTRIBUTING.md
Mar 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ Before new components and patterns are published into the design system, the tea
| [Tested](#tested) | It’s been tested and shown to work with a range of browsers, assistive technologies and devices. |
| [Considered](#considered) | Documentation and rationale have been provided. |

When making a new component, we would be very grateful if you post the research, design decisions and use cases for the component. Accessibility considerations should be documented or sourced from the [community forum](https://community.digital.gov.au/c/designsystem) before submitting a pull request.

This may be in the form of a code snippet, screenshots, sketch files or written text on your research with references. This gives a chance for members of the community to respond and share any work they may have done in the past on a similar component.

-------------------------------------------------------------------------------------------------

### Useful
Expand Down Expand Up @@ -164,6 +168,80 @@ Provide rationale; the more the better. We aim to explain design and code decisi

-------------------------------------------------------------------------------------------------

### Code Standard

As mentioned previously, we try to make our code very easy to understand and readable for the human user. Here are some do's and don'ts

#### Describe the function

Make sure to include function and parameter definitions where appropriate. The name of the function should help make clear the intention of the function. This standard applies for any SASS, JS, JQuery or React JS functions. We adhere to [JSDoc](http://usejsdoc.org/about-getting-started.html) styling

Don't:

```js
function CalculateSpecs(initialSize,endSize,speed){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

...
}
```

Do

```js
* @param {integer} initialSize - The initial size of the element to animate
* @param {integer} endSize - The size the element after the animation completes
* @param {string} speed - The speed of the animation in ms
*
* @return {object} - Required steps, stepSize and intervalTime for the animation
*/
function CalculateAnimationSpecs( initialSize, endSize, speed ) {
...
}

```

#### Space out the code

Add space when there is code between round, square or curly brackets.

Don't:

```scss
$isBlackBgContrast:AU-color-contrast(#000,$background,true,false)>=$ratio;

```

Do:

```scss
$isBlackBgContrast: AU-color-contrast( #000, $background, true, false ) >= $ratio;
```

#### Use new lines
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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} 
>
  1. 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} >

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@azerella azerella Mar 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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} >

When HTML or React tags have several attributes (or props), it can be easier to separate these onto a new line.

Don't:

```jsx
<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' : '' }` } >
```

Do:
```jsx
<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' : '' }`
} >
```

**[⬆ back to top](#contents)**


-------------------------------------------------------------------------------------------------

## Reporting Bugs, Asking Questions, Sending Suggestions

Expand Down