-
Notifications
You must be signed in to change notification settings - Fork 0
wip: add demo topper definition #73
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?
Conversation
README.md
Outdated
| type: 'topper' | ||
| topperType: TopperType | ||
| backgroundColor: string // maybe a type? is this external?? | ||
| children: [Headline, Intro, TopperVisual?] |
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.
should these be keys rather than children? allows for further growth?
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 think it does make sense.
the benefit of having them as children i guess would be if there wasn't a regimented structure, and the FE could receive/render anything (like we do with RichText).
this probably isn't true for the topper
| suggestedBackgroundColor: string // this is what editorial select, but can be overridden based on other content properties | ||
| headline: Headline | ||
| intro: Intro | ||
| visual: CustomCodeComponent | ImageSet // | ClipSet |
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.
Flourish and Picture, that are the two visual supported at the moment, are in reality in two different position in the markup. We can look to unify them and keep them both as Visual or we may need an additional field (That doesn't seem right).
| interface Headline extends Parent { | ||
| type: 'headline' | ||
| children: Text[] | ||
| external isLarge: boolean //is this external? it's based on some business logic, partly derived by topper type? should it be external? |
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.
Currently hardcode in the model .
It maps 121 with an Origami prop so this is potentially an information that other consumers may use too. If that the case, it should not come from our model, however, putting the mapping that is in the model in CAPI doesn't seem right to me, let's discuss
| type: 'topper' | ||
| suggestedTopperLayout: TopperLayout | ||
| suggestedBackgroundColor: string // this is what editorial select, but can be overridden based on other content properties | ||
| headline: Headline |
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.
Following the idea to let the FE to decide the Headline we can keep this as a string
What is this?
This draft PR is a loose implementation of what a topperTree could look like. Ignore the details - it's mostly a place to start the conversation about feasibility and desirability.
Why do I think it's desirable?
bodyTreehas served us well for creating a common and consistent language to describe the full representation of an article body, and the data required for it.Toppers do not have the same luxury - the structure of a topper (in Spark/C&M) has been largely unchanged since 2017. Data in C&M lives in a bunch of fields: core fields like
headline,standfirst, some things intopper,leadImages,leadFlourish. There's also a lot hidden of inferred decisions made in business logic in CP.We're now looking to add (clips and/or custom components) to toppers, and it seems like a nice opportunity to start thinking about whether we can apply some of the same thinking and reap the same benefits with toppers, to result in a more consistent, reusable data models.
How will it work (in theory)?
topperTreedata structuretopperTreetransit data structure to C&MtopperTreedata structure from the new C&M read API (which we'll also be doing forbodyTree)What are some of the questions/challenges?
externalstill make sense?backgroundColor)