Skip to content
This repository has been archived by the owner on Sep 8, 2021. It is now read-only.

Add Design.CorrectionsAndClarifications #164

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

abeddow91
Copy link
Contributor

@abeddow91 abeddow91 commented Aug 3, 2021

Why?

There's some unique styling for Corrections and Clarifications on Editions that we'd like to capture via Design, specifically that this design shouldn't feature as byline as this provides no value on Editions. Adding this would also allow us to uniquely style this on on dotcom and apps too, if required.

Changes

  • Add Design.Correction

Screenshot

| |

Copy link
Contributor

@oliverlloyd oliverlloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pro using Design for these types of branches but just to check: Have we considered if we need the design branch on Editions? Could we instead align things more?

The other side to that question is, if we defo want this design change, then should it also be done in Dotcom and Apps?

src/format.ts Outdated Show resolved Hide resolved
@abeddow91 abeddow91 force-pushed the add-corrections-and-clarifications branch from c4d8465 to 0f1ce43 Compare August 3, 2021 11:30
@abeddow91
Copy link
Contributor Author

abeddow91 commented Aug 3, 2021

Have we considered if we need the design branch on Editions? Could we instead align things more?

Yeah, we have considered that. The reason we saw for not being able to align is that, on Apps and Dotcom, the byline is clickable and links to the list of previous Corrections and Clarifications (they're published roughly everyday). On Editions however, this isn't clickable meaning we get Corrections and Clarifications repeated 3 times in the header but with no added value to the repetition. I've added a screenshot above to show how this looks.

NB - there's a known bug in ER bullet points at the moment.

src/format.ts Outdated
@@ -34,6 +34,7 @@ enum Design {
PhotoEssay,
PrintShop,
Obituary,
Corrections,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing I'd add here is that whilst it might not be needed necessarily at the Scala client level, there'll need to be an update to both the Scala content client and by extension the FAPI client to include this new type.

It's annoying but it's best to keep them in sync for now until the unified upstream type stuff using Thrift can come into play

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Josh! Do you know who the best person is to chat to about those changes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably me... Though I'm leaving in a couple of days. There are some docs in the Dotcom shared drive about it. The FAPI change is something for the Dotcom team to do, so don't worry about that. It's just a case of bumping the content client version in the FAPI client and then bumping both the content client and FAPI client in Frontend.

The content client change itself is something along these lines: guardian/content-api-scala-client#333

General steps are:

  1. Go into the Design (or other type) bit of the CAPI Enrichment and add the condition for the new design to the predicates list. Note: Things earlier in the list get higher priority so make sure you consider what should take priority when placing it.
  2. Add a test for the new type. If there's a specific condition where it's likely it could be one of two types and this one should take priority, add a test for that too e.g. this example here
  3. Do a release of the content client. You'll need a CAPI key in order to do this because it runs tests during the release process. CAPI team can help with this.

Let me know if you want any help with it. If you make the change today or tomorrow I should be free to review/pair on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff, thanks!

@JamieB-gu
Copy link
Contributor

then should it also be done in Dotcom and Apps?

I think there is a good argument that it should - probably a question for Editorial & Design. @paperboyo any thoughts?

the byline is clickable and links to the list of previous

This is true, although both apps and dotcom also have a clickable series for these pieces which should link to the same list. The byline is more or less duplicating that functionality.

@SiAdcock
Copy link
Contributor

SiAdcock commented Aug 3, 2021

Couple of quick points, not blocking:

  1. We should probably make the same changes to Format in @guardian/libs
  2. Is there anything that's stopping apps-rendering from consuming Format from @guardian/libs?

@paperboyo
Copy link

which should link to the same list

Slightly different numbers for both lists (probs some unbylined?).

My thought would be that it isn’t worth the effort to treat this specific column differently. It also makes Design dependent to series or byline (doesn’t matter which), which seems flaky/specific.
Agreed that something repeat 3 times looks off and it looks more off in Editions where two are not clickable and the article itself may be more prominent that on the web/apps. Is it off enough to warrant adding extra logic? Not sure, but not my call.

@JamieB-gu
Copy link
Contributor

2. Is there anything that's stopping apps-rendering from consuming Format from @guardian/libs?

The renaming mainly I think - it's going to be a biggish find-and-replace job. We might be able to simplify by having a proxy module that exports the old names.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants