Storybook helpers v5 #1714
Storybook helpers v5 #1714
Conversation
ryanmccombe
requested review from
dr3,
j-pendlebury,
pjlee11 and
sareh
as code owners
| slots, | ||
| componentFunction, | ||
| services, | ||
| options = {}, |
There was a problem hiding this comment.
are we expecting more options? what if this arg was just defaultService = 'news'
There was a problem hiding this comment.
Yeah, ide almost prefer
(slots, componentFunction, services)
->
({ slots, componentFunction, services, defaultService })
or put services inside options, as its optional
There was a problem hiding this comment.
I don't know if we'll need to add more arguments to this eventually, but I think the amount of arguments here is already problematic - you can see that in the test file where there are null parameters being passed in everywhere
Made the 4th an options object to stop the problem getting any further out of hand - https://github.com/ryanmcdermott/clean-code-javascript#function-arguments-2-or-fewer-ideally
There was a problem hiding this comment.
@dr3 agree that all the arguments should just be one object eventually, but that's a massive breakign change that would require updating all stories in Simorgh and Psammead to a new API. Not sure that's in scope for media pod? We just want our page on storybook 😢
There was a problem hiding this comment.
@ryanmccombe definitely not in scope for the media pod or this PR really. Could you raise an issue about that refactor though so it can be picked up and looked into by the core pod at a later date.
There was a problem hiding this comment.
| @@ -3,6 +3,7 @@ | |||
| <!-- prettier-ignore --> | |||
| | Version | Description | | |||
| |---------|-------------| | |||
| | 5.0.0 | [PR#1714](https://github.com/bbc/psammead/pull/1714) Renamed `indonesian` to `indonesia` and `afaanOromoo` to `afaanoromoo`. Input provider now has an optional 4th argument that allows the default service to be configured | | |||
There was a problem hiding this comment.
Any reason you went with 5.0.0 rather than 4.1.0, IMO this doesn't need to be a major change
There was a problem hiding this comment.
I believe the service rename part of this is a breaking change - previously, "indonesian" appeared on the dropdown, and got passed to your component if someone selected it. Now, it will be "indonesia"
At least I think that's true - the only thing that makes me doubt it is surely some components would have errored if you passed an invalid service name in the current version?
But maybe nobody noticed it as these services weren't used
There was a problem hiding this comment.
Ahhh yeah, it is likely those services haven't been used/bugs haven't been spotted.
|
No testing needed here - just some behind-the-scenes work to add functionality that will be used for future PRs |
Resolves #1668
Overall change: Update service names for parity with Simorgh, and add option to set default service.
Code changes:
indonesiantoindonesia.afaanOromootoafaanoromoo.inputProvidernow allows a default service to be configured, rather than always usingnews