holidayextras / ui-toolkit

DEPRECATED - CSS & React components
http://tech.holidayextras.co.uk/ui-toolkit/
MIT License
14 stars 3 forks source link

APP-267 - allowing spread operator on main tile div #145

Closed lukehansell closed 8 years ago

lukehansell commented 8 years ago

What does this PR do? (please provide any background)

In using the toolkit components it has come to light that we need to be more flexible in our approach. It's great giving people a framework, but when it's as rigid as this tile it isn't flexible to all the required uses. For instance the grouped product tiles require an onClick handler. To add in a new PR to add an extra prop/attribute to the tile every time we want to slightly change something or add a mouseOver handler seems nonsense.

Therefore we're adding the spread operator so that, within the confines of it's application we can customise our use of the tile without having to add extra code to the toolkit.

I wanted to use the rest destructuring to do something like var { image, children, ...props } = this.props but that wasn't possible with the make-up rules and eslint's support (or intentional lack of) for ES7. I therefore used lodash's omit to achieve the same result (props without the image or children). After some investigation I've found that the lodash method is faster to render in Tripapp than the rest method and the difference between the polyfill and lodash is negligible.

Also: https://facebook.github.io/react/docs/transferring-props.html#transferring-with-underscore I'm aware the above link is talking about if we don't use JSX, which we do, but ES7 destructuring is subject to change/removal and we need something stable to achieve this. Hence lodash appears to me to be the better option.

I did look at doing this without lodash or the destructuring, but the props are immutable and read only so even if I cloned them I'd still have the same issue.

What tests does this PR have?

I've added a test to check that a spread prop will be passed through.

How can this be tested?

run the docs and check that you can add an onClick or equivalent to the tile.

Screenshots / Screencast

What gif best describes how you feel about this work?


Review 1

By adding a +1 you are confirming you have...

ghost commented 8 years ago

@lukehansell, thanks for your PR! By analyzing the blame information on this pull request, we identified @jodiedoubleday, @robhuzzey and @jackdcrawford to be potential reviewers

elliottcrush commented 8 years ago

Thanks for the detailed explanation, happy with your investigation and action. This approach has had plenty of discussion prior to undertaking the work so happy to go through without a 2nd review. Nice work @lukehansell 👍