-
Notifications
You must be signed in to change notification settings - Fork 30
Football match goal attempts #15014
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
base: main
Are you sure you want to change the base?
Football match goal attempts #15014
Conversation
a8fbdd8 to
3c1cb7f
Compare
54d000d to
fbd32a1
Compare
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
fbd32a1 to
7e2a85c
Compare
| background-color: var(--on-target-colour); | ||
| border-radius: 4px; | ||
| width: 80%; | ||
| min-height: 62px; |
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.
This slightly magical number reflects the design, without seeming to conform to our layout rules – assuming they are applicable in this case, which I accept they might not be.
As a consequence on desktop, because the font for the number of attempts is larger, you don't get as much space below the text.
mobile
desktop
I wonder if we should think in terms of how much space we want to have below the number of attempts, rather than the size of this box? If not, we could at least specify a different number for min-height here. For instance min-height: 72px at desktop looks like:
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'm not a fan of magic numbers in general, but the minimum height applied here is per design. It's meant to sit alongside the larger numbers on desktop, hence the addition of a negative margin to shift the box up slightly. I can have another look at this as I would like to have a layout that's more fluid and intrinsic, and less reliant on magic numbers.
|
A few comments / questions but looking good in general, and I like the refactor and extraction of the For my own curiosity, is the process now to get a pair of design eyes on this at the component level through a Chromatic review, or do we do a design review more holistically? |
We haven't done any design reviews yet as things are still in progress and there will be further tweaks as the individual components get assembled into complete pages and we see everything together. Once we're further ahead it would be good to do a design run through with Rich and send him some links to Chromatic / Storybook so he can review at his leisure. |
What does this change?
Adds
FootballMatchGoalAttemptscomponent and refactors existingFootballMatchStatcomponent to enable sharing of the base layout. (As part of this the existing layout override props have been consolidated into a singlelayoutprop, and team details broken out into separate props.)Why?
To allow us to show stats for goal attempts on the new match summary page (#14904)
To do
Adjust text colour so it has sufficient contrast with the background which is derived from team colours. (Currently the text colours are hardcoded to match the example team colours in Storybook.) This is captured in #14987
Screenshots