bbc / simorgh

The BBC's Open Source Web Application. Contributions welcome! Used on some of our biggest websites, e.g.
https://www.bbc.com/pidgin
Other
1.42k stars 225 forks source link

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

Closed LilyL0u closed 3 days ago

LilyL0u commented 1 week ago

Resolves JIRA [number]

Overall changes

A very high-level summary of easily-reproducible changes that can be understood by non-devs, and why these changes where made.

Code changes

Testing

  1. List the steps used to test this PR.

Helpful Links

Add Links to useful resources related to this PR if applicable.

Coding Standards

Repository use guidelines

LilyL0u commented 1 week ago

@amoore108 I need to increase the max bundle size for the article page by 1kb apparently. Should I do this by doing export const MAX_SIZE = 1175 + 6; or export const MAX_SIZE = 1175 + 5;

or is there another way of preventing this?

amoore108 commented 1 week ago

@amoore108 I need to increase the max bundle size for the article page by 1kb apparently. Should I do this by doing export const MAX_SIZE = 1175 + 6; or export const MAX_SIZE = 1175 + 5;

or is there another way of preventing this?

The output says:

Largest total bundle size (kB) (largest service + largest page)    │ 1181 |

Curious why its gone up with this change. There are quite a few diffs to the ArticlePage component though, are they all needed? Looks like linting and console logs.

The blocks component as well can be reverted back to its original state before this PR.

I'd make those changes before adjusting the bundle size to see if that helps.

amoore108 commented 1 week ago
│ Smallest total bundle size (kB) (smallest service + smallest page) │ 643  │
├────────────────────────────────────────────────────────────────────┼──────┤
│ Largest total bundle size (kB) (largest service + largest page)    │ 1179 │
└────────────────────────────────────────────────────────────────────┴──────┘

This is from my PR, so I would change the MIN/MAX values in the bundleSizeConfig to the values from the build output in this PR. The +5 should stay as thats a buffer.

Its likely just slowly creeped up and is now getting caught by these min/max values.

LilyL0u commented 1 week ago

@amoore108 I need to increase the max bundle size for the article page by 1kb apparently. Should I do this by doing export const MAX_SIZE = 1175 + 6; or export const MAX_SIZE = 1175 + 5; or is there another way of preventing this?

The output says:

Largest total bundle size (kB) (largest service + largest page)    │ 1181 |

Curious why its gone up with this change. There are quite a few diffs to the ArticlePage component though, are they all needed? Looks like linting and console logs.

The blocks component as well can be reverted back to its original state before this PR.

I'd make those changes before adjusting the bundle size to see if that helps.

I have removed the remaining dregs from Blocks and the console log from Article Page, but there is still the bundle size issue. In the comparison of the file in github it looks like loads of changes to the file, but almost all of it isn't actually a change and is just the code is on different lines due to adding

const showRelatedContent = blocks.some(
  block => block.type === 'relatedContent',
);

which shifts things down. Makes it hard to see the real differences, but there aren't actually many!

LilyL0u commented 1 week ago
│ Smallest total bundle size (kB) (smallest service + smallest page) │ 643  │
├────────────────────────────────────────────────────────────────────┼──────┤
│ Largest total bundle size (kB) (largest service + largest page)    │ 1179 │
└────────────────────────────────────────────────────────────────────┴──────┘

This is from my PR, so I would change the MIN/MAX values in the bundleSizeConfig to the values from the build output in this PR. The +5 should stay as thats a buffer.

Its likely just slowly creeped up and is now getting caught by these min/max values.

Screenshot 2024-11-12 at 13 38 47

changing bundleSize based on this output 👍

amoore108 commented 1 week ago

I'm getting quite a bit of linting errors with the ArticlePage:

Screenshot 2024-11-12 at 13 44 44
LilyL0u commented 1 week ago

I'm getting quite a bit of linting errors with the ArticlePage: Screenshot 2024-11-12 at 13 44 44

I don't see these in my vscode 🤔 Maybe the IDE isn't showing me linting errors properly? I'll format the document and see if that helps

amoore108 commented 1 week ago

Still a bit concerned about https://github.com/bbc/simorgh/pull/12157#discussion_r1836945584, we're focusing on the span, not the related content h2 which I think will be impactful.