-
Notifications
You must be signed in to change notification settings - Fork 234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DNM] Sport Tables #9166
[DNM] Sport Tables #9166
Changes from 1 commit
16677f8
9f118a4
a4e7be1
f846631
434bc96
d9f5c83
4c80d9f
4ec03c7
461004e
8b6e45e
3e8c850
c02f1e2
1808969
e5f2967
9690993
b10b87c
c312c63
27ec8e2
936528c
78dbea7
3670241
42feb42
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,20 @@ | ||
import React from 'react'; | ||
import styled from '@emotion/styled'; | ||
import { arrayOf, shape } from 'prop-types'; | ||
import { arrayOf, shape, string } from 'prop-types'; | ||
import { GEL_SPACING } from '@bbc/gel-foundations/spacings'; | ||
|
||
import Blocks from '../../Blocks'; | ||
import CpsText from '../../CpsText'; | ||
|
||
const StyledTd = styled.td` | ||
border-bottom: 1px solid #dbdbdb; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These colours come from the existing Sport colours here - https://github.com/bbc/gs-sass-tools/blob/master/settings/_sport-colours.scss#L14 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 :) |
||
padding-top: 8px; | ||
padding-bottom: 8px; | ||
padding-left: 8px; | ||
padding-top: ${GEL_SPACING}; | ||
padding-bottom: ${GEL_SPACING}; | ||
padding-left: ${GEL_SPACING}; | ||
vertical-align: middle; | ||
|
||
${({ isHeaderCell }) => isHeaderCell && 'background: #f7f7f5;'} | ||
|
||
& p { | ||
padding-bottom: 0; | ||
} | ||
|
@@ -21,16 +24,22 @@ const componentsToRender = { | |
text: CpsText, | ||
}; | ||
|
||
const CpsTableCell = ({ blocks }) => { | ||
const CpsTableCell = ({ blocks, type }) => { | ||
const isHeaderCell = type === 'tableHeader'; | ||
return ( | ||
<StyledTd> | ||
<StyledTd as={isHeaderCell ? 'th' : 'td'} isHeaderCell={isHeaderCell}> | ||
<Blocks blocks={blocks} componentsToRender={componentsToRender} /> | ||
</StyledTd> | ||
); | ||
}; | ||
|
||
CpsTableCell.propTypes = { | ||
blocks: arrayOf(shape({})).isRequired, | ||
type: string, | ||
}; | ||
|
||
CpsTableCell.defaultProps = { | ||
type: 'tableCell', | ||
}; | ||
|
||
export default CpsTableCell; |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree! #4478