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

Wsteama 1417 add related content link to jump to component #12157

Open
wants to merge 38 commits into
base: latest
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
63597ee
related content but with content shift, for educational purposes only
LilyL0u Nov 11, 2024
40a18d5
add related content heading to jumpToHeadings array
LilyL0u Nov 11, 2024
205bb6f
Merge branch 'latest' into WSTEAMA-1417-add-related-content-link-to-j…
LilyL0u Nov 11, 2024
0f320c1
translation for related content
LilyL0u Nov 11, 2024
c39f71c
Merge branch 'WSTEAMA-1417-add-related-content-link-to-jump-to-compon…
LilyL0u Nov 11, 2024
79238d5
translation for most read
LilyL0u Nov 11, 2024
6f8e6bf
tests fixed and one adapted to also check for related content and mos…
LilyL0u Nov 11, 2024
a015006
test for when related content is not present
LilyL0u Nov 11, 2024
16951a0
related content in storybook
LilyL0u Nov 11, 2024
a4d8100
Merge branch 'latest' into WSTEAMA-1417-add-related-content-link-to-j…
LilyL0u Nov 12, 2024
432c727
remove most read and some bits like log removal
LilyL0u Nov 12, 2024
079522e
Merge branch 'WSTEAMA-1417-add-related-content-link-to-jump-to-compon…
LilyL0u Nov 12, 2024
39f9061
some lint fixes
LilyL0u Nov 12, 2024
0552f15
Merge branch 'latest' into WSTEAMA-1417-add-related-content-link-to-j…
LilyL0u Nov 12, 2024
0e622ac
decide related content to show in article page and send into component
LilyL0u Nov 12, 2024
43ea66c
Merge branch 'WSTEAMA-1417-add-related-content-link-to-jump-to-compon…
LilyL0u Nov 12, 2024
9fcf10f
change tests for new way
LilyL0u Nov 12, 2024
732d57e
update storybook to match logic
LilyL0u Nov 12, 2024
cee44d0
positive test for jumpTo component on article page
LilyL0u Nov 12, 2024
1bb89d4
Merge branch 'latest' into WSTEAMA-1417-add-related-content-link-to-j…
LilyL0u Nov 12, 2024
fa1e02c
update snapshot
LilyL0u Nov 12, 2024
6e39739
remove unused translations
LilyL0u Nov 12, 2024
63d4cb4
some clean up
LilyL0u Nov 12, 2024
7238908
increase bundle size for article page
LilyL0u Nov 12, 2024
51a4037
lint
LilyL0u Nov 12, 2024
330be29
more lint
LilyL0u Nov 12, 2024
f7e0770
data-testid for related content is same format as others
LilyL0u Nov 12, 2024
63a7a59
Merge branch 'latest' into WSTEAMA-1417-add-related-content-link-to-j…
LilyL0u Nov 12, 2024
f3386eb
article page tests for jump to component
LilyL0u Nov 12, 2024
3ecb949
remove arialabelledby id, corresponding id, snapshot, and other bits
LilyL0u Nov 12, 2024
0494f88
remove arialabelled by and id from related content link, and snapshot
LilyL0u Nov 12, 2024
dfd2f14
remove console log
LilyL0u Nov 12, 2024
d401bbb
add related content to jumptToHeadings with sanitised id and change t…
LilyL0u Nov 12, 2024
6e04892
change duplicate variable name
LilyL0u Nov 12, 2024
2ade0d3
Merge branch 'latest' into WSTEAMA-1417-add-related-content-link-to-j…
LilyL0u Nov 14, 2024
ffb09bc
Merge branch 'latest' into WSTEAMA-1417-add-related-content-link-to-j…
amoore108 Nov 15, 2024
31214b8
remove snapshot test
LilyL0u Nov 15, 2024
516db9e
related content heading id change
LilyL0u Nov 15, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/bundleSize/bundleSizeConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@
*/

export const MIN_SIZE = 635 - 5;
export const MAX_SIZE = 1175 + 5;
export const MAX_SIZE = 1181 + 5;
23 changes: 20 additions & 3 deletions src/app/components/JumpTo/index.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,16 @@ import JumpTo, { JumpToProps } from '.';
import metadata from './metadata.json';
import readme from './README.md';

const Component = ({ jumpToHeadings = [] }: JumpToProps) => {
return <JumpTo jumpToHeadings={jumpToHeadings} />;
const Component = ({
jumpToHeadings = [],
showRelatedContentLink = false,
}: JumpToProps) => {
return (
<JumpTo
jumpToHeadings={jumpToHeadings}
showRelatedContentLink={showRelatedContentLink}
/>
);
};

export default {
Expand All @@ -23,5 +31,14 @@ export const Example = () => {
block => block.type === 'jumpTo',
)?.model.jumpToHeadings;

return <Component jumpToHeadings={jumpToHeadings} />;
const showRelatedContentLink =
!!pidginArticleFixtureWithJumpToBlock.data.article.content.model.blocks.find(
block => block.type === 'relatedContent',
);
return (
<Component
jumpToHeadings={jumpToHeadings}
showRelatedContentLink={showRelatedContentLink}
/>
);
};
Comment on lines +34 to 44
Copy link
Contributor

@amoore108 amoore108 Nov 12, 2024

Choose a reason for hiding this comment

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

Worth deleting the blocks.find stuff and just having a separate story for the related content link?

Like:

 <Component
      jumpToHeadings={jumpToHeadings}
      showRelatedContentLink={false}
    />
...
 <Component
      jumpToHeadings={jumpToHeadings}
      showRelatedContentLink
    />

27 changes: 16 additions & 11 deletions src/app/components/JumpTo/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ describe('JumpTo Component', () => {
jest.clearAllMocks();
});

const jumpToHeadings = jumpToBlock?.model.jumpToHeadings ?? [];
const jumpToHeadings: Array<{ heading: string; sanitisedId?: string }> =
jumpToBlock?.model.jumpToHeadings ?? [];

describe('Render jumpTo', () => {
it('renders the Jump To title', () => {
Expand All @@ -29,19 +30,23 @@ describe('JumpTo Component', () => {
expect(title).toBeInTheDocument();
});

it('renders the correct number of headings', () => {
it('renders the correct number of headings without related content link', () => {
render(<JumpTo jumpToHeadings={jumpToHeadings} />);
const headings = screen.getAllByRole('listitem');
expect(headings.length).toBe(jumpToHeadings.length);
});
it('renders the correct number of headings with related content link', () => {
render(<JumpTo jumpToHeadings={jumpToHeadings} showRelatedContentLink />);
const headings = screen.getAllByRole('listitem');
expect(headings.length).toBe(jumpToHeadings.length);
});

it('renders each item with a link to the corresponding subheading on the same page', () => {
render(<JumpTo jumpToHeadings={jumpToHeadings} />);
const listItems = screen.getAllByRole('listitem');
listItems.forEach((item, index) => {
const link = item.querySelector('a');
const sanitisedHeading = idSanitiser(jumpToHeadings[index].heading);
const expectedHref = `#${sanitisedHeading}`;
render(<JumpTo jumpToHeadings={jumpToHeadings} showRelatedContentLink />);
jumpToHeadings.forEach(({ heading, sanitisedId }) => {
const id = sanitisedId || idSanitiser(heading);
const link = screen.getByTestId(`jump-to-link-${id}`);
const expectedHref = `#${id}`;
expect(link).toHaveAttribute('href', expectedHref);
});
});
Expand Down Expand Up @@ -76,9 +81,9 @@ describe('JumpTo Component', () => {

render(<JumpTo jumpToHeadings={jumpToHeadings} />);

jumpToHeadings.forEach(({ heading }) => {
const sanitisedId = idSanitiser(heading);
const link = screen.getByTestId(`jump-to-link-${sanitisedId}`);
jumpToHeadings.forEach(({ heading, sanitisedId }) => {
const id = sanitisedId || idSanitiser(heading);
const link = screen.getByTestId(`jump-to-link-${id}`);

fireEvent.click(link);
expect(link.onclick).toBeTruthy();
Expand Down
27 changes: 17 additions & 10 deletions src/app/components/JumpTo/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,24 +12,24 @@ import idSanitiser from '../../lib/utilities/idSanitiser';
import styles from './index.styles';

export interface JumpToProps {
jumpToHeadings?: Array<{ heading: string }>;
jumpToHeadings?: Array<{ heading: string; sanitisedId?: string }>;
showRelatedContentLink?: boolean;
}

const eventTrackingData: EventTrackingMetadata = {
componentName: 'jumpto',
};

const JumpTo = ({ jumpToHeadings }: JumpToProps) => {
const JumpTo = ({ jumpToHeadings, showRelatedContentLink }: JumpToProps) => {
// TODO: Remove for release
if (isLive()) return null;

const { translations } = useContext(ServiceContext);
const [hash, setHash] = useState('');
const { jumpTo = 'Jump to' } = translations?.articlePage || {};

const relatedContent = translations?.relatedContent || 'Related content';
const viewRef = useViewTracker(eventTrackingData);
const clickTrackerHandler = useClickTrackerHandler(eventTrackingData);

useEffect(() => {
setHash(window.location.hash);
}, []);
Expand All @@ -43,7 +43,15 @@ const JumpTo = ({ jumpToHeadings }: JumpToProps) => {
};

const titleId = 'jump-to-heading';

if (
showRelatedContentLink &&
!jumpToHeadings?.some(({ heading }) => heading === relatedContent)
Copy link
Contributor

@amoore108 amoore108 Nov 12, 2024

Choose a reason for hiding this comment

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

Will this part of the condition ever be hit? Or was there a re-rendering thing happening whereby its was adding relatedContent to the array multiple times?

If thats happening, we should try not to mutate the original data, instead taking a copy of the jumpToHeadings and then pushing the relatedContent item to the copy, not the original.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it was adding the link again each rerender. I have been working on trying the copy of the array in storybook as it happens there too. I will do the same in this code too when I have got it working 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea it may be good to try and construct an array of 'things to render', including the related content if applicable, so something like:

  const headingsToRender = [
    // Article subheadings
    ...jumpToHeadings.map(({ heading }) => ({
      heading,
      id: idSanitiser(heading),
    })),
    // Related content
    showRelatedContentLink && {
      heading: relatedContent,
      id: 'section-label-heading-related-content-heading',
    },
  ];

That'll take a shallow copy of the jumpToHeadings, so shouldn't cause the original data to be mutated and keep appending items to it on re-renders.

) {
jumpToHeadings?.push({
heading: relatedContent,
sanitisedId: 'related-content-heading',
amoore108 marked this conversation as resolved.
Show resolved Hide resolved
});
}
return (
<nav
ref={viewRef}
Expand All @@ -62,18 +70,17 @@ const JumpTo = ({ jumpToHeadings }: JumpToProps) => {
{jumpTo}
</Text>
<ol role="list" css={styles.list}>
{jumpToHeadings?.map(({ heading }) => {
const sanitisedId = idSanitiser(heading);
const idWithHash = `#${sanitisedId}`;

{jumpToHeadings?.map(({ heading, sanitisedId }) => {
const id = sanitisedId || idSanitiser(heading);
const idWithHash = `#${id}`;
const isActiveId = decodeURIComponent(hash) === idWithHash;
return (
<li key={idWithHash} css={styles.listItem}>
<a
href={idWithHash}
onClick={e => linkClickHandler(e, idWithHash)}
css={styles.link}
data-testid={`jump-to-link-${sanitisedId}`}
data-testid={`jump-to-link-${id}`}
>
<span
css={[styles.linkText, isActiveId && styles.linkTextActive]}
Expand Down
1 change: 0 additions & 1 deletion src/app/legacy/containers/Blocks/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ const Blocks = ({ blocks, componentsToRender }) =>
if (!componentsToRender || !type) {
return null;
}

const Block = componentsToRender[type];

if (!Block) {
Expand Down
10 changes: 6 additions & 4 deletions src/app/pages/ArticlePage/ArticlePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ import {

const ArticlePage = ({ pageData }: { pageData: Article }) => {
const { isApp, pageType, service, isAmp, id } = useContext(RequestContext);

const {
articleAuthor,
isTrustProjectParticipant,
Expand Down Expand Up @@ -102,7 +101,6 @@ const ArticlePage = ({ pageData }: { pageData: Article }) => {
const topics = pageData?.metadata?.topics ?? [];
const blocks = pageData?.content?.model?.blocks ?? [];
const startsWithHeading = blocks?.[0]?.type === 'headline' || false;

const bylineBlock = blocks.find(
block => block.type === 'byline',
) as OptimoBylineBlock;
Expand Down Expand Up @@ -148,7 +146,9 @@ const ArticlePage = ({ pageData }: { pageData: Article }) => {
service,
id,
});

const showRelatedContent = blocks.some(
block => block.type === 'relatedContent',
);
const componentsToRender = {
visuallyHiddenHeadline,
headline: headings,
Expand Down Expand Up @@ -196,7 +196,9 @@ const ArticlePage = ({ pageData }: { pageData: Article }) => {
topStoriesContent ? (
<ExperimentTopStories topStoriesContent={topStoriesContent} />
) : null,
jumpTo: JumpTo,
jumpTo: (props: ComponentToRenderProps) => (
<JumpTo {...props} showRelatedContentLink={showRelatedContent} />
),
};

const visuallyHiddenBlock = {
Expand Down
Loading
Loading