Feature/article canonical urls #1378
Conversation
nehasri89
force-pushed
the
feature/article-canonical-urls
branch
from
43a0907
to
2e2b7b4
Compare
nehasri89
force-pushed
the
feature/article-canonical-urls
branch
from
2e2b7b4
to
1cde2d3
Compare
|
Please find visual snapshots of your changed components here: https://s3-eu-west-1.amazonaws.com/times-components-snaps/1cde2d39aa29f31a7b53323d89008df63127259b/index.html |
| import articleListFixture from "../../fixtures/articles.json"; | ||
| import adConfig from "../../fixtures/article-ad-config.json"; | ||
| import ArticleList from "../../src/article-list"; | ||
| import { makeUrl } from "./../utils"; |
There was a problem hiding this comment.
could be just "../utils"
| error={apolloError} | ||
| refetch={() => {}} | ||
| /> | ||
| <Context.Provider value={{ makeUrl }}> |
There was a problem hiding this comment.
Why don't we add this as a default value instead?
There was a problem hiding this comment.
The reason I did not put it in the default is that then it becomes the default way to do it, whereas a noop is just saying this is just a default and client's responsible for providing it
| @@ -15,6 +16,9 @@ import { | |||
| import storybookReporter from "@times-components/tealium-utils"; | |||
| import AuthorProfile from "./src/author-profile"; | |||
|
|
|||
| const makeUrl = ({ slug, shortIdentifier }) => | |||
| `https://www.thetimes.co.uk/article/${slug}-${shortIdentifier}`; | |||
There was a problem hiding this comment.
I'd prefer a default value in context package so we can dry these up
There was a problem hiding this comment.
at least move the function to the utils package
|
Please find visual snapshots of your changed components here: https://s3-eu-west-1.amazonaws.com/times-components-snaps/de3b2f9b584f308da52d3d3b1b7bd310cb51746b/index.html |
| @@ -19,3 +19,6 @@ export const omitNative = new Set([ | |||
| ]); | |||
|
|
|||
| export const omitWeb = new Set(["className", "data-testid", "style"]); | |||
|
|
|||
| export const makeUrl = ({ slug, shortIdentifier }) => | |||
There was a problem hiding this comment.
can this be moved to the utils package so it can be reused (e.g. by the showcase, and by the article package)
| @@ -15,6 +16,9 @@ import { | |||
| import storybookReporter from "@times-components/tealium-utils"; | |||
| import AuthorProfile from "./src/author-profile"; | |||
|
|
|||
| const makeUrl = ({ slug, shortIdentifier }) => | |||
| `https://www.thetimes.co.uk/article/${slug}-${shortIdentifier}`; | |||
There was a problem hiding this comment.
at least move the function to the utils package
| @@ -17,6 +18,9 @@ import opinionAndTwo1RelatedArticleFixture from "./fixtures/opinionandtwo/1-arti | |||
| import opinionAndTwo2RelatedArticlesFixture from "./fixtures/opinionandtwo/2-articles.js"; | |||
| import opinionAndTwo3RelatedArticlesFixture from "./fixtures/opinionandtwo/3-articles.js"; | |||
|
|
|||
| const makeUrl = ({ slug, shortIdentifier }) => | |||
| `https://www.thetimes.co.uk/article/${slug}-${shortIdentifier}`; | |||
| @@ -0,0 +1,138 @@ | |||
| import React from "react"; | |||
There was a problem hiding this comment.
this whole file is just a duplicate of related-article-item.js. Please create a related-article-item-base.js file and compose what you need out of that to keep the code DRY
There was a problem hiding this comment.
The plan is to do the same change for the native side pretty soon, so the web file lives only until then. Therefore I would be tempted to make minimal changes for now
There was a problem hiding this comment.
I disagree. we either maintain device parity, or keep the codebase DRY and maintainable. Ideally both? @craigbilner to weigh-in?
packages/topic/topic.showcase.js
Outdated
| import storybookReporter from "@times-components/tealium-utils"; | ||
| import Topic from "./src/topic"; | ||
| import TopicProvider from "../provider/src/topic"; | ||
| import adConfig from "./fixtures/topic-ad-config.json"; | ||
|
|
||
| const makeUrl = ({ slug, shortIdentifier }) => | ||
| `https://www.thetimes.co.uk/article/${slug}-${shortIdentifier}`; |
|
Please find visual snapshots of your changed components here: https://s3-eu-west-1.amazonaws.com/times-components-snaps/31f4ca618a2807c9aa7038bdf805406296066575/index.html |
|
WIP |
This PR allows Author Profile and Topics articles to have env agnostic URLs. Also RA will have env agnostic URLs too.
There are issues with Context API and enzyme shallow rendering enzymejs/enzyme#1647
enzymejs/enzyme#1513
So therefore we need to rethink about tests that are failing in Related Articles where we use Context API and Shallow render.