-
Notifications
You must be signed in to change notification settings - Fork 30
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
Handle crossword articles #12925
Handle crossword articles #12925
Conversation
Size Change: +12.7 kB (+1.45%) Total Size: 883 kB
ℹ️ View Unchanged
|
0789e74
to
8a244f7
Compare
2e7e45f
to
3fe07f7
Compare
this is a first pass, it needs work
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.
Looks good! A few questions/suggestions.
# Conflicts: # dotcom-rendering/src/types/content.ts # dotcom-rendering/src/types/frontend.ts
…s and out of handle.article.web
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.
Some follow up items but no blockers 👍
@@ -50,6 +50,7 @@ | |||
"@guardian/libs": "19.2.1", | |||
"@guardian/ophan-tracker-js": "2.2.5", | |||
"@guardian/react-crossword": "2.0.2", | |||
"@guardian/react-crossword-next": "npm:@guardian/react-crossword@0.0.0-canary-20250114144251", |
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.
Would be preferable to use a release version rather than a canary, particularly before we move to a >0% size test.
@@ -126,6 +127,7 @@ export interface FEArticleType { | |||
showTableOfContents: boolean; | |||
lang?: string; | |||
isRightToLeftLang?: boolean; | |||
crossword?: CrosswordProps['data']; |
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 is a larger point but I was surprised to see crossword
as its own top-level property of the Frontend data model rather than as standard 'block'.
Something we can rationalise as it would mean we don't have to parse out the data separately in the enhancers and can treat as any other element type?
Co-authored-by: Oliver Abrahams <ollie.abrahams@guardian.co.uk>
Seen on PROD (created by @sndrs and merged by @oliverabrahams 7 minutes and 43 seconds ago) Please check your changes! |
based heavily on the work done by @andrew-nowak in #12511, this is first pass of getting crosswords working in DCR.
it does not include layout changes, because there's work to be done in CAPI client and frontend to support that.
those will come in #13001.