[DNM] Sport Tables #9166
[DNM] Sport Tables #9166
Conversation
ryanmccombe
added
the
cross-team
src/app/routes/cpsAsset/getInitialData/convertToOptimoBlocks/blocks/table/index.js
Show resolved
Hide resolved
…ort-table-plumbing
GPG Test
| @@ -0,0 +1,48 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Just wondering why CpsTable is in the containers directory rather than components; tbh I'm not clear what the distinction is in Simorgh myself? The distinction I've come across tends involve where the react component does data fetching or not. So if the CpsTable had a useEffect to get some data it might be more suitable as a container, if it is just a dumb component hydrated with data via props it is more of a component? I'm unsure if the distinction is that useful in our codebase but just want to try and be consitent where we can.
There was a problem hiding this comment.
Yeah I don't find the distinction useful - I think the whole "containers and components" pattern was popularised by Dan Abramov in a 6-year-old medium post that went viral: https://medium.com/@dan_abramov/smart-and-dumb-components-7ca2f9a7c7d0
He has since updated it to say he doesn't consider it a useful pattern - I tend to agree. Particularly in Simorgh, it seems somewhat arbitrary where we put components. Reason I put this one in containers is really just because that's where all the other block renderers are
There was a problem hiding this comment.
Thanks @ryanmccombe, just wanted to sound this out, I'd be in favour flattening the structure to just components we should bring this up with the wider team. Just thinking about this stuff before we do live pages where we will make a lot more components.
I'd stick with this in containers like you've done for now 👍
| import CpsText from '../../CpsText'; | ||
|
|
||
| const StyledTd = styled.td` | ||
| border-bottom: 1px solid #dbdbdb; |
There was a problem hiding this comment.
Should these colours reference values from https://github.com/bbc/psammead/blob/latest/packages/utilities/psammead-styles/src/colours.js
Just in case we are using colours inconsistent with those agreed by UX?
There was a problem hiding this comment.
These colours come from the existing Sport colours here - https://github.com/bbc/gs-sass-tools/blob/master/settings/_sport-colours.scss#L14
Should I open a PR to add these two colours to psammead?
There was a problem hiding this comment.
yeah it's a bit annoying but that's our source of truth for colours, just makes clear what UX etc refer to them as and allows us to change them en masse e.g. for chameleon project.
Feel free to do this in a follow-up PR though
There was a problem hiding this comment.
Sorry was looking into this earlier and got distracted - I did some comparison and it seems the colour here is different to what is on the sport pages like https://www.bbc.co.uk/sport/cricket/54646061
That page seems to have a border-bottom of #eee rather than #dbdbdb - the hover background colour seems to be slightly different too
Not sure if it's an issue. Either way, none of these exact colours were on our pallette. If we wanted to use the variables we'd need to either add new ones to psammead, or update the components to use one of the existing variables if they're "close enough"
Happy to help with either of these :)
There was a problem hiding this comment.
Ah good spot! I actually copied these colours from AlephAMP rather than Morph - https://www.bbc.co.uk/sport/amp/cricket/54646061
I'll open a follow up PR to add these to psammead :)
| }; | ||
|
|
||
| const CpsTable = ({ blocks, supportedServices }) => { | ||
| const { service } = useContext(ServiceContext); |
There was a problem hiding this comment.
Ah the use of the contexts and stuff here makes this feel more like a container, it is pulling in Simorgh specific constructs and sanitising the data for rendering. Again unsure if the distiniction is useful just wanted to open the discussion out.
Maybe CpsTable should be a container while CpsTableRow and CpsTableCell are more components?
There was a problem hiding this comment.
The reason I put CpsTableRow and CpsTableCell inside the CpsTable folder was because they'll only ever be used as children of a CpsTable component. I'm very new to Simorgh though so maybe this was isn't conventional! I'm happy to move them to components if that's recommended?
There was a problem hiding this comment.
The makes total sense @paulcrane-bbc , we're working this stuff out a bit for ourselves at minute see: #9166 (comment)
I'd leave this as you have done it, it makes perfect sense given we don't have a clear definition of components vs containers.
|
Do we have an asset on test to use for testing when this is ready? I could add a Cypress test for tables on amp Sport pages If we don't have one maybe I can make one |
|
Sorry I clicked 'close with comment' instead of 'comment' 😱 |
|
Hey @LilyL0u 👋 There's also this asset in live if it's ok to use that environment - https://www.test.bbc.co.uk/sport/cricket/54646061.amp?renderer_env=live |
Do not merge yet - Sport in high scrutiny because of Euros
Resolves NEWSWORLDSERVICE-1252
Overall change:
Implement support for CPS Tables within Story pages, required for Sport
Code changes:
cross-teamlabel to this PR if it requires visibility across World Service teamsTesting:
CYPRESS_APP_ENV=local CYPRESS_SMOKE=false yarn test:e2e:interactive)