Skip to content
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

Feature/article canonical urls #1378

Closed
wants to merge 30 commits into from
Closed

Conversation

nehasri89
Copy link

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.

@nehasri89 nehasri89 force-pushed the feature/article-canonical-urls branch from 43a0907 to 2e2b7b4 Compare October 3, 2018 11:00
@nehasri89 nehasri89 force-pushed the feature/article-canonical-urls branch from 2e2b7b4 to 1cde2d3 Compare October 3, 2018 13:29
@times-tools
Copy link
Collaborator

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";
Copy link
Contributor

Choose a reason for hiding this comment

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

could be just "../utils"

Copy link
Contributor

@tuncaulubilge tuncaulubilge left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Got only one suggestion around adding a default makeUrl function, which would simplify most of the showcase and test changes

error={apolloError}
refetch={() => {}}
/>
<Context.Provider value={{ makeUrl }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we add this as a default value instead?

Copy link
Author

Choose a reason for hiding this comment

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

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}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer a default value in context package so we can dry these up

Copy link
Contributor

Choose a reason for hiding this comment

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

at least move the function to the utils package

@times-tools
Copy link
Collaborator

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 }) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

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}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

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}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

move to utils package

@@ -0,0 +1,138 @@
import React from "react";
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree. we either maintain device parity, or keep the codebase DRY and maintainable. Ideally both? @craigbilner to weigh-in?

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}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

utils package

@times-tools
Copy link
Collaborator

Please find visual snapshots of your changed components here: https://s3-eu-west-1.amazonaws.com/times-components-snaps/31f4ca618a2807c9aa7038bdf805406296066575/index.html

@craigbilner
Copy link
Contributor

WIP

@craigbilner craigbilner closed this Oct 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants