Skip to content

Commit 9ec01b6

Browse files
author
Juarez Mota
committed
Merge branch 'jm/new-sign-in-gate' of github.com:guardian/dotcom-rendering into jm/new-sign-in-gate
2 parents 33307d3 + 109767d commit 9ec01b6

40 files changed

+555
-377
lines changed

README.md

Lines changed: 5 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,33 +1,11 @@
1-
# Dotcom/Apps Rendering
1+
# Dotcom Rendering & Apps Rendering
22

3-
This repository contains the rendering logic for articles on theguardian.com. It is a monorepo with 2 projects, `apps-rendering` and `dotcom-rendering`.
3+
This repository contains the rendering logic for theguardian.com and for a subset of articles in the live apps.
44

5-
## Developer setup
6-
7-
Install [Node.js](https://nodejs.org).
8-
9-
We recommend using [fnm](https://github.com/Schniz/fnm). It is great at managing multiple versions of Node.js on one machine.
10-
11-
> You may find it useful to add `--version-file-strategy recursive` to the [`fnm` shell setup](https://github.com/Schniz/fnm?tab=readme-ov-file#shell-setup). This will set the active Node version to first version it finds in the current directory _or_ any parent directory.
12-
13-
Once Node is installed, make sure you're using the correct package manager by [enabling corepack](https://github.com/nodejs/corepack?tab=readme-ov-file#utility-commands):
14-
15-
```sh
16-
corepack enable
17-
```
18-
19-
> [!NOTE]
20-
>
21-
> If you're using `asdf`, you'll need to run `asdf reshim nodejs` after running `corepack enable`.
22-
23-
## Install
24-
25-
Run `pnpm install` in the root directory of this project to install packages.
5+
It is a monorepo with 2 projects, `apps-rendering` and `dotcom-rendering`.
266

277
## Run
288

29-
You should always `cd` into the correct subdirectory before running commands (e.g `make dev` for dotcom-rendering, or `pnpm watch` for apps-rendering) except for storybook.
30-
319
### `apps rendering`
3210

3311
Go to [apps rendering](apps-rendering/README.md) for more details.
@@ -38,15 +16,9 @@ Go to [dotcom rendering](dotcom-rendering/README.md) for more details.
3816

3917
## Root actions
4018

41-
Most commands are run from within each project but the following are managed from the monorepo root:
19+
Most commands are run from within each project but the following can be run from the root:
4220

43-
### Storybook/Chromatic
21+
### Storybook
4422

4523
`pnpm storybook` - Runs Storybook for all projects
4624
`pnpm build-storybook` - Builds Storybook for all projects
47-
48-
Chromatic now runs at project level. `cd` into the project dir and run `pnpm chromatic -t [CHROMATIC PROJECT TOKEN]`
49-
50-
You can find the token in the project Chromatic instance.
51-
52-
To run Chromatic in CI on your pr, add the `run_chromatic` label once you're ready to check for visual regressions.

dotcom-rendering/README.md

Lines changed: 134 additions & 72 deletions
Large diffs are not rendered by default.

dotcom-rendering/docs/contributing/detailed-setup-guide.md

Lines changed: 1 addition & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -10,40 +10,7 @@ This high level diagram shows the difference between the data flow when DCR is u
1010

1111
## Getting started
1212

13-
To download the repository
14-
15-
```
16-
$ git clone [email protected]:guardian/dotcom-rendering.git
17-
$ cd dotcom-rendering
18-
```
19-
20-
### Node.js
21-
22-
Make sure you have [Node.js](https://nodejs.org) installed.
23-
24-
We recommend using [fnm](https://github.com/Schniz/fnm) to help manage multiple versions of Node.js on on machine.
25-
26-
### Install Dependencies
27-
28-
run
29-
30-
```
31-
$ make install
32-
```
33-
34-
If it complains that you do not have the right version of node, then run (or replace with the correct version manager or the correct version):
35-
36-
```
37-
$ fnm install 22.14.0
38-
```
39-
40-
### Running on local
41-
42-
```sh
43-
$ make dev
44-
```
45-
46-
This will start the development server on port 3030: [http://localhost:3030](http://localhost:3030).
13+
See [Getting Started](../../README.md#getting-started)
4714

4815
### Previewing article on local
4916

@@ -55,16 +22,6 @@ You can use this technique to integrate with a locally running instance of `fron
5522

5623
http://localhost:3030/Article/http://localhost:9000/world/2013/jun/09/edward-snowden-nsa-whistleblower-surveillance
5724

58-
### Previewing AMP on local
59-
60-
You can preview an AMP page similarly to an article, as follows
61-
62-
http://localhost:3030/AMPArticle/https://amp.theguardian.com/world/2013/jun/09/edward-snowden-nsa-whistleblower-surveillance
63-
64-
or, connecting to a locally running instance of frontend,
65-
66-
http://localhost:3030/AMPArticle/http://localhost:9000/world/2013/jun/09/edward-snowden-nsa-whistleblower-surveillance
67-
6825
### Note on rebasing vs merging
6926

7027
The dotcom-rendering github account is set up to merge PRs into main instead of rebase. Merge commits are useful to quickly revert things when there is a major incident - whereas with rebase you might have to revert a whole load of commits.
328 KB
Loading
233 KB
Loading

dotcom-rendering/docs/testing.md

Lines changed: 72 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
1-
# Testing your JavaScript
1+
# Testing
22

3-
## Jest tests
3+
## Unit tests
44

5-
Tests are run using [Jest](https://jestjs.io). You can run the full unit test suite locally by running:
5+
Unit tests are run using [Jest](https://jestjs.io).
6+
7+
You can run the full unit test suite locally by running:
68

79
```bash
810
make test
@@ -26,50 +28,55 @@ Alternatively, you can use `watch` mode to have jest run the suite as files are
2628
pnpm test --watch
2729
```
2830

29-
## Writing tests
31+
### Location
3032

31-
Tests should be colocated with the module/Component under test and should end with the suffix `.test.ts` or `.test.tsx`.
33+
Unit tests should be colocated with the module under test and should end with the suffix `.test.ts` or `.test.tsx`.
3234

3335
For example:
34-
`components/ShareCount.tsx`
35-
`components/ShareCount.test.tsx`
3636
`lib/cookie.ts`
3737
`lib/cookie.test.ts`
3838

39-
## React Testing Library tests
39+
## Component tests
4040

41-
To test components we use [react-testing-library](https://github.com/kentcdodds/react-testing-library). This a testing utility that allows you to tests against the rendered DOM, output from your component. The utility makes you write tests from a user-perspective, asserting against user-facing properties.
41+
To test components we use [react-testing-library](https://github.com/testing-library/react-testing-library). It allows you to tests agains the rendered DOM output of your component. The library makes you write tests from a "user-perspective", asserting against user-facing properties.
4242

43-
### Do test
43+
### Location
4444

45-
When we write tests for Components, we should test the render logic and not the internal implementation details.
45+
Component tests should be colocated with the component under test and should end with the suffix `.test.tsx`.
4646

47-
Test the expected behavior of a component: what it renders, how the props it receives alter what's rendered and how user interactions and events that trigger changes of state alter what's rendred.
47+
For example:
48+
`components/ShareCount.tsx`
49+
`components/ShareCount.test.tsx`
4850

49-
Look for conditional statements (ternaries, if statements, and switch statements) in the render method for clues about what to test. You should always test your public interface (the props your component takes). For example, if the Component under test takes a prop which is a function that is executed onClick we can test it’s executed by simulating a click on the Component under test.
51+
## Visual tests
5052

51-
### Don't test
53+
We use [Storybook](https://storybook.js.org/) and [Chromatic](https://www.chromatic.com/) to create visual regression tests.
5254

53-
Tests that require you to duplicate the application code exactly. These tests will be brittle. For example if a component renders with a height 10px, if you test that the component has a height 10px and you then update the Component so it’s height is 11px then you’ll have to update your test, even though the component’s behavior has not changed. Don’t test inline styles, unless the value can change based on props/state.
55+
To run Storybook locally:
5456

55-
Don't assert against behaviours covered by the library code. EG. Testing types of properties passed to the component, this is unnecessary.
57+
`pnpm storybook`
5658

57-
### Snapshots
59+
To run Chromatic locally:
5860

59-
Jest [Snapshot](https://jestjs.io/docs/en/snapshot-testing) tests make sure the rendered output of a UI Component does not change unexpectedly.
61+
`pnpm chromatic -t [CHROMATIC PROJECT TOKEN]`
6062

61-
We will **not** be writing snaphot test Components for the following reasons:
63+
You can find the token in the project Chromatic instance.
6264

63-
- Snapshots immortalize a Component’s current markup as the true markup, regardless of whether or not the component is correct.
64-
- Too easy to fix tests without fixing underlying bugs simplu by running `jest --u` override command.
65-
- Increased risk of false negatives and false positives - If the tests fail when there are no bugs, that is a false negative. If the tests pass when there are bugs present, that is a false positive.
66-
- Developer time required to check snapshot test failures when simple non-breaking changes introduced, plus developer time required to review snapshot output in Pull Requests.
65+
To run Chromatic in CI on your pr, add the `run_chromatic` label once you're ready to check for visual regressions.
6766

68-
## Playwright tests
67+
### Location
68+
69+
Storybook tests should be colocated with the component(s) under test and should end with the suffix `.stories.tsx`.
70+
71+
For example:
72+
`components/Avatar.tsx`
73+
`components/Avatar.stories.tsx`
6974

70-
[Playwright](https://playwright.dev/) offers a solution for integration tests where tests are executed in a real browser. By executing at this level it provides a realistic representation of a user interacting with the page.
75+
## E2E tests
7176

72-
### How to run locally
77+
[Playwright](https://playwright.dev/) offers a solution for E2E tests where tests are executed in a real browser.
78+
79+
Only create a Playwright test when a full browser environment is required.
7380

7481
The first time you run Playwright you will be asked to install a local browser, which you can do with the following command:
7582

@@ -81,8 +88,45 @@ To run the tests on the command line in headless mode:
8188

8289
`make playwright-run`
8390

84-
To open the tests in the Playwright UI:
91+
To open the tests in the Playwright UI, with hot reloading:
8592

8693
`make playwright-open`
8794

88-
Changes to the tests and implementation code will hot reload.
95+
### Location
96+
97+
Playwright tests are located in the [playwright](../playwright/) directory and should end with the suffix `e2e.spec.ts`.
98+
99+
For example:
100+
`lightbox.e2e.spec.ts`
101+
102+
## Testing strategy
103+
104+
### Testing pyramid
105+
106+
Understand the testing pyramid:
107+
https://circleci.com/blog/testing-pyramid/
108+
109+
### Do test
110+
111+
When we write tests for Components, we should test the render logic and not the internal implementation details.
112+
113+
Test the expected behaviour of a component: what it renders, how the props it receives alter what's rendered and how user interactions and events that trigger changes of state alter what's rendered.
114+
115+
Look for conditional statements (ternaries, if statements, and switch statements) in the render method for clues about what to test. You should always test your public interface (the props your component takes). For example, if the Component under test takes a prop which is a function that is executed onClick we can test it’s executed by simulating a click on the Component under test.
116+
117+
### Don't test
118+
119+
Tests that require you to duplicate the application code exactly. These tests will be brittle. For example if a component renders with a height 10px, if you test that the component has a height 10px and you then update the Component so its height is 11px then you’ll have to update your test, even though the component’s behaviour has not changed. Don’t test inline styles, unless the value can change based on props/state.
120+
121+
Don't assert against behaviours covered by the library code. EG. Testing types of properties passed to the component, this is unnecessary.
122+
123+
### Snapshots
124+
125+
Jest [Snapshot](https://jestjs.io/docs/en/snapshot-testing) tests make sure the rendered output of a UI Component does not change unexpectedly.
126+
127+
We will **not** be writing snapshot test Components for the following reasons:
128+
129+
- Snapshots immortalise a Component’s current markup as the true markup, regardless of whether or not the component is correct.
130+
- Too easy to fix tests without fixing underlying bugs simply by running `jest --u` override command.
131+
- Increased risk of false negatives and false positives - If the tests fail when there are no bugs, that is a false negative. If the tests pass when there are bugs present, that is a false positive.
132+
- Developer time required to check snapshot test failures when simple non-breaking changes introduced, plus developer time required to review snapshot output in Pull Requests.

dotcom-rendering/src/components/AdSlot.web.tsx

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ type DefaultProps = {
4747
display?: ArticleDisplay;
4848
isPaidContent?: boolean;
4949
hasPageskin?: boolean;
50+
isIn250ReservationVariant?: boolean;
5051
};
5152

5253
// for dark ad labels
@@ -106,12 +107,29 @@ const hideBelowDesktop = css`
106107
}
107108
`;
108109

110+
const containerMinHeight = getMinHeight(250, space[5]);
111+
109112
const topAboveNavContainerStyles = css`
110-
padding-bottom: 18px;
113+
padding-bottom: ${space[5]}px;
114+
position: relative;
115+
margin: 0 auto;
116+
text-align: left;
117+
display: block;
118+
`;
119+
120+
const topAboveNavContainerVaraintStyles = css`
121+
padding-bottom: ${space[5]}px;
111122
position: relative;
112123
margin: 0 auto;
113124
text-align: left;
114125
display: block;
126+
width: 100%;
127+
min-height: ${containerMinHeight}px;
128+
129+
/* Remove the min-height when the ad has rendered, so that the container can shrink if the ad is smaller */
130+
&[top-above-nav-ad-rendered='true'] {
131+
min-height: auto;
132+
}
115133
`;
116134

117135
/**
@@ -412,6 +430,7 @@ export const AdSlot = ({
412430
index,
413431
hasPageskin = false,
414432
shouldHideReaderRevenue = false,
433+
isIn250ReservationVariant,
415434
}: Props) => {
416435
switch (position) {
417436
case 'right':
@@ -590,7 +609,13 @@ export const AdSlot = ({
590609
}
591610
case 'top-above-nav': {
592611
return (
593-
<AdSlotWrapper css={topAboveNavContainerStyles}>
612+
<AdSlotWrapper
613+
css={
614+
isIn250ReservationVariant
615+
? topAboveNavContainerVaraintStyles
616+
: topAboveNavContainerStyles
617+
}
618+
>
594619
<div
595620
id="dfp-ad--top-above-nav"
596621
className={[

dotcom-rendering/src/components/Card/Card.stories.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1565,7 +1565,7 @@ export const WithATinyGap = () => {
15651565
imagePositionOnDesktop="left"
15661566
format={{
15671567
display: ArticleDisplay.Standard,
1568-
design: ArticleDesign.Gallery,
1568+
design: ArticleDesign.Standard,
15691569
theme: Pillar.Sport,
15701570
}}
15711571
/>

0 commit comments

Comments
 (0)