Skeleton OnDemand Radio Episode page #5804
Conversation
| import pipe from 'ramda/src/pipe'; | ||
| import path from 'ramda/src/path'; | ||
| import map from 'ramda/src/map'; | ||
| import mergeDeepLeft from 'ramda/src/mergeDeepLeft'; | ||
| import uuid from 'uuid'; | ||
|
|
||
| const addIdToBlock = block => ({ ...block, uuid: uuid() }); | ||
|
|
||
| const getBlocks = path(['content', 'blocks']); | ||
|
|
||
| const mapBlocks = pipe(getBlocks, map(addIdToBlock)); | ||
|
|
||
| export default jsonRaw => | ||
| mergeDeepLeft( | ||
| { | ||
| content: { blocks: mapBlocks(jsonRaw) }, | ||
| }, | ||
| jsonRaw, | ||
| ); |
There was a problem hiding this comment.
This appears to be a duplicate of https://github.com/bbc/simorgh/blob/latest/src/app/routes/radio/getInitialData/addIdsToBlocks.js - is it possible to try and reuse this somehow?
There was a problem hiding this comment.
You could pop it in here src/app/routes/utils/sharedDataTransformers and call it addIdsToRadioBlocks so both live radio and ondemand routes can use it
There was a problem hiding this comment.
Not sure if this is in scope for this PR as it's just supposed to be a blank page, but I'm not really sure we need to be using the Blocks renderer for this page at all in the long term
It seems to be the case that ARES payloads for these pages only ever have one block - the block that contains the episode info
We don't have to be iterating over blocks if there's only ever one block, and if we're not iterating over blocks, we don't need to be adding IDs to them
src/app/containers/RadioPageBlocks/Blocks/OnDemandRadio/index.jsx
Outdated
Show resolved
Hide resolved
| OnDemandRadioPage.propTypes = { | ||
| pageData: shape({ | ||
| metadata: shape({ | ||
| id: string, | ||
| tags: object, | ||
| }), | ||
| promo: shape({ | ||
| subtype: string, | ||
| name: string, | ||
| }), | ||
| content: shape({ | ||
| blocks: arrayOf( | ||
| shape({ | ||
| uuid: string, | ||
| id: string, | ||
| externalId: string, | ||
| text: string, | ||
| type: string, | ||
| }), | ||
| ), | ||
| }), | ||
| }).isRequired, | ||
| }; |
There was a problem hiding this comment.
Is it worth extracting this into a separate function like this: https://github.com/bbc/simorgh/blob/latest/src/app/models/propTypes/radioPage/index.js
There was a problem hiding this comment.
I've stressed so many times how important it is to keep prop types explicit and inline with the component and not to abstract them away because they it's super handy documentation for developers:
#4477
https://gist.github.com/jroebu14/cb3f82f51d74a02a586e7bc430414a05
https://gist.github.com/jroebu14/0ff63ba198e7e46cfce2df3381710256
I'd prefer it if we kept them as is even if it means duplication. The problem is really that our prop type definitions are too complex because we pass components the full Ares response instead of picking out only the bits we need from the response before passing props down. An example of how I'd like our prop types to be https://github.com/bbc/simorgh/pull/5566/files#diff-e449f3ce3c0cbe3fd43e2629cf8661baR71-R76 rather having a hidden away centralised and hard to understand system of proptypes.
We should do this for all our components. I refactored the Metadata component a while ago to accept only the props it needs instead of the full Ares response and how look how simple it is to use now! https://github.com/bbc/simorgh/pull/5804/files#diff-6f9218412f92864ee8850958a05c23a2R21-R24 I want to do the same for the ATI component #5743
Sorry for all the reading but I'm passionate about this issue because it's about developer experience! Grab me if you want me to explain more.
There was a problem hiding this comment.
Excellent explanation - thank you. Maybe it would make sense to eventually delete the functions in /src/app/models/propTypes/ which are no longer needed or used anymore. (But not in this PR obvs!)
| .add('default', ({ service }) => ( | ||
| <BrowserRouter> | ||
| <OnDemandRadioPage | ||
| match={matchFixtures(service)} | ||
| pageData={onDemandRadioFixtures[service]} | ||
| status={status} | ||
| service={service} | ||
| isAmp={false} | ||
| loading={false} | ||
| error="" | ||
| pageType="media" | ||
| /> | ||
| </BrowserRouter> | ||
| )); |
There was a problem hiding this comment.
Could we add a second example here for amp?
RESOLVES #5797
Overall change:
Adding a new page type for OnDemand radio episodes
Code changes:
Testing:
CYPRESS_APP_ENV=local CYPRESS_SMOKE=false npm run test:e2e:interactive)