Skip to content

Change dotcoom rendering parameter (?guui => ?dcr=true/false) #21753

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

Merged
merged 2 commits into from
Aug 21, 2019

Conversation

philmcmahon
Copy link
Contributor

@philmcmahon philmcmahon commented Aug 20, 2019

What does this change?

Renames the parameter we use to force dotcom rendering. Currently we have guui and guui=false. In this bright new world we will have dcr=true and dcr=false which seems more sensible.

It's still an acronym though so I'm happy for something else, just not sure rendering= would be that good either. I've stuck a comment in where the value is read from the query string, hopefully that should minimise confusion.

Tested

  • Locally
  • On CODE (optional)

@PRBuilds
Copy link

PRBuilds commented Aug 20, 2019

PRbuilds results:

Screenshots
wide.pngdesktop.pngtablet.pngmobile.png

💚 A11y validation
a11y-report.txt

💚 Microdata Validation
microdata.txt

Apache Benchmark Load Testing
loadtesting.txt

LightHouse Reporting

--automated message

@@ -78,7 +78,7 @@ class ArticleController(
}

private def getJson(article: ArticlePage)(implicit request: RequestHeader): List[(String, Object)] = {
val contentFieldsJson = if (request.isGuuiJson) List(
Copy link
Contributor

@MatthewJWalls MatthewJWalls Aug 20, 2019

Choose a reason for hiding this comment

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

It looks pretty correct to get rid of guuiJson to just be using forceDCR.

But just to make 100% sure, can you check that .json?dcr=true on an article definitely still works locally?, and produces a different result to .json?dcr=false, which should be equivalent to just .json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep have verified this!

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@philmcmahon philmcmahon force-pushed the pm-migrate-dcr-parameter branch from e59759f to 8521163 Compare August 20, 2019 15:53
@philmcmahon philmcmahon merged commit 36978f8 into master Aug 21, 2019
@philmcmahon philmcmahon deleted the pm-migrate-dcr-parameter branch August 21, 2019 13:47
@prout-bot
Copy link
Collaborator

Seen on PROD (merged by @philmcmahon 19 minutes and 12 seconds ago)

rtyley added a commit to guardian/dotcom-rendering that referenced this pull request Aug 18, 2020
guardian/frontend#21753 made `?dcr` the way to
control whether or not a given article is rendered by DCR, but this was
not documented in DCR's README.md!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants