Skip to content
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

Implement Card Composable #726

Closed
mutebg opened this issue Mar 22, 2023 · 12 comments · Fixed by #802
Closed

Implement Card Composable #726

mutebg opened this issue Mar 22, 2023 · 12 comments · Fixed by #802
Assignees
Labels
story a user facing change
Milestone

Comments

@mutebg
Copy link
Contributor

mutebg commented Mar 22, 2023

Description

We need to implement Card Composable component

Requirements

  • Compoent to be named CardComposable
  • CardMedia currently takes media={imageProps || React.Node },we should use children instead of React.Node for CardComposbale
  • CardActions need to build on top of GridLayout instead of Block, in that way every Card-sub-component will have the same API
  • Create a default areas prop on the CardComposable component
  • Update some of the stories, eg Some of what's on the feature branch's "Overrides" story might need moving to a "Styling overrides" story if custom weskit storybook themes required for it.

Business Benefit/Value

Clear business reason stating why this piece of work is important to the business/technology. Any clear KPI's or goals it is attempting to meet. Any other known tangible or intangible benefits

Acceptance Criteria

All acceptance criteria must be met before task can be marked as 'done'

Design

Design handoff

Supporting Information

Process/sequence flows, wireframes, third party documentation, tech feasibility, architecture overview/documentation, high level design, data specs etc...

Dependencies

Any known business dependencies or reasons to engage with another team and input into their roadmap/timings

@mutebg mutebg added the triage This issue will be reviewed by the team label Mar 22, 2023
@jps jps added this to NewsKit Mar 27, 2023
@jps jps added this to the Card V2 milestone Mar 27, 2023
@jps jps moved this to Analysis in NewsKit Mar 28, 2023
@JohnTParsons JohnTParsons changed the title (Story) - Implement Card Composable Implement Card Composable Mar 30, 2023
@LukeFinch LukeFinch added task A non user facing change story a user facing change and removed task A non user facing change labels Mar 30, 2023
@LukeFinch LukeFinch moved this from Analysis to Ready to do in NewsKit Mar 30, 2023
@mutebg mutebg removed the triage This issue will be reviewed by the team label Mar 31, 2023
@mutebg mutebg moved this from Ready to do to In progress in NewsKit Mar 31, 2023
@mutebg mutebg self-assigned this Mar 31, 2023
@mutebg mutebg mentioned this issue Apr 4, 2023
8 tasks
@mutebg mutebg linked a pull request Apr 4, 2023 that will close this issue
8 tasks
@mutebg
Copy link
Contributor Author

mutebg commented Apr 4, 2023

@mutebg mutebg moved this from In progress to Design review in NewsKit Apr 4, 2023
@mutebg
Copy link
Contributor Author

mutebg commented Apr 4, 2023

few more things:
For the CardLink I am using LinkStandalone which comes with its own stylePreset linkStandalone, is that ok?

Also, when I use LinkStandalone it makes whole focus looks like a block, is that the desired behaviour or prefer the default one
Image

Image

@GeriReid
Copy link
Contributor

GeriReid commented Apr 4, 2023

Thanks @mutebg, looks great. To feed back on your points:

  • Agree with your Figma comments - fine to remove the on Click and second colour overrides story
  • Styling overrides
    Colour example - I've changed background to interfaceinformative020 copy to inkBrand010 from the Storybook theme. This should work on light and dark.
  • LinkStandalone looks good for the focus state
  • Your fun story makes me want to go on holiday! Good demo, I'd leave it in

Only other spot was in the Span story

  • 2:1 horizontal ratio - the height of the tag increases at a wider browser width, is that intended?

@mutebg
Copy link
Contributor Author

mutebg commented Apr 5, 2023

All comments addressed.

@GeriReid
Copy link
Contributor

GeriReid commented Apr 5, 2023

Morning @mutebg, thanks for the updates.
Just one query on the tags. Some change height depending on browser width. Looks like something to do with grid? Could you make them all a consistent height?
image

Rest looks good to me, let me know if you spot anything else @nathanparris

@mutebg
Copy link
Contributor Author

mutebg commented Apr 5, 2023

@GeriReid I can fix that, found the issue, according to the spec CardActions needs to have minHeight: sizing080 (48px) and since we use a CSS grid it automatically stretches the tag, so 2 things:

  • I will change the default behavior not to stretch it
  • Do we need this default min-height for the actions, isn't it better if the CardActions height depends on the content inside?

@GeriReid
Copy link
Contributor

GeriReid commented Apr 5, 2023

@mutebg only thought on the min-height is accessibility but we should be able to rely on our consumers to make sensible choices. Makes sense that CardActions height should depend on the content inside it.

Do you think we should change the default behaviour or just roll with what the browser gives us - consumers can choose to override if they want to?

@nathanparris
Copy link
Contributor

@mutebg Just a couple of notes from me. The rest is looking good

Whole card as a link by applying the 'expand' prop

  • Use 3x2 image

Different layouts/font sizes for different breakpoints

  • Add overrides for Headline to make the story changes more obvious xl:'editorialHeadline050', xs:'editorialHeadline030'
  • Unordered list - change xl to editorialParagraph020

I'm not sure what the min-height on actions container is really for. Maybe it was added so that all cards have the same height in a section?

@mutebg
Copy link
Contributor Author

mutebg commented Apr 5, 2023

IMO we should remove the min-height from CardActions, and remove the default stretching behaviour, which will feel more natural way of behaviour.
I would not worry about a11y in that case because consumers always can put very small buttons with small hit area if they want, so we can't restrict them.

@nathanparris Thanks will update these.
I believe the min-height is taken from the old Card component as many of other defaults in the spec which are not very relevant

@mutebg
Copy link
Contributor Author

mutebg commented Apr 5, 2023

@nathanparris @GeriReid all this updated

@GeriReid
Copy link
Contributor

GeriReid commented Apr 5, 2023

Looks good to me, thanks for turning this around so quickly @mutebg. Okay with you @nathanparris?

@nathanparris
Copy link
Contributor

@GeriReid @mutebg All good with me

@mutebg mutebg moved this from Design review to Peer review in NewsKit Apr 5, 2023
@mutebg mutebg moved this from Peer review to Ready to deploy in NewsKit Apr 11, 2023
@newskit-engineering newskit-engineering moved this from Ready to deploy to Done in NewsKit Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
story a user facing change
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants