-
Notifications
You must be signed in to change notification settings - Fork 210
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 1157 - Decommission Most Watched Page Delete Simorgh Code #11731
WSTEAMA 1157 - Decommission Most Watched Page Delete Simorgh Code #11731
Conversation
…his still happen?
return 'h2'; | ||
} | ||
|
||
export const getHeadingTagOverride = ({ isContentTypeGuide }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully editors will no longer put links to most watched in story promos (if this is what that is), as they will lonk to 404s but also they will not have the right heading or a11y attributes anymore.
Note: if we change our mind and want the component actually not deleted, the last commit before I started deleting most watched component stuff is: 'remove most watched from bundle size tests'. So we can revert to here if we do change our minds. |
@@ -13,8 +13,8 @@ import fetchPageData from '.'; | |||
|
|||
const expectedBaseUrl = 'http://localhost'; | |||
const requestedPathname = '/path/to/asset'; | |||
const fullTestPath = 'https://test.bbc.com/hausa/mostwatched.json'; | |||
const fullLivePath = 'https://www.bbc.com/hausa/mostwatched.json'; | |||
const fullTestPath = 'https://test.bbc.com/hausa/mostread.json'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The urls here don't actually matter if they work on test or live, the test just checks the correct url is passed through. So I could have left it as most watched, but it might confuse future people who see 'most watched' when that is no longer a thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically most read paths don't exist either... but for the purposes of this test I suppose it doesn't matter? Or should we consider using a more bff-like path? e.g. https://test.mock-bff.api.bbc.com/simorgh-bff?pageType=bob?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added that one :)
.vscode/settings.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this file was supposed to be in version control?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleted
@@ -320,8 +320,7 @@ | |||
"secondaryData": { | |||
"topStories": null, | |||
"features": null, | |||
"mostRead": null, | |||
"mostWatched": null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reminds me: we should also delete the mostWatched fetching from the BFF to truly clean this up
MOST_WATCHED_PROCESS_ERROR: 'most_watched_process_error', | ||
MOST_WATCHED_STALE_DATA: 'most_watched_stale_data', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could potentially remove these queries from the Cloudwatch: https://github.com/bbc/simorgh-infrastructure/blob/latest/packages/cloudwatch-insights-queries/lib/index.ts#L288-L308
const MostWatchedWrapper = styled.div` | ||
padding-bottom: ${GEL_SPACING_QUAD}; | ||
`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is likely to be the reason for the Chromatic diff - padding at the bottom that is no longer required 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent, that's fine then :)
@@ -128,14 +126,6 @@ export default async ({ | |||
throw handleError('CPS asset data fetch error', status); | |||
} | |||
|
|||
const { mostWatched } = processMostWatched({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you haven't already, can we make sure that this function is also deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find anything else left when I search 'processMostWatched', so hopefully I have
@@ -13,8 +13,8 @@ import fetchPageData from '.'; | |||
|
|||
const expectedBaseUrl = 'http://localhost'; | |||
const requestedPathname = '/path/to/asset'; | |||
const fullTestPath = 'https://test.bbc.com/hausa/mostwatched.json'; | |||
const fullLivePath = 'https://www.bbc.com/hausa/mostwatched.json'; | |||
const fullTestPath = 'https://test.bbc.com/hausa/mostread.json'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically most read paths don't exist either... but for the purposes of this test I suppose it doesn't matter? Or should we consider using a more bff-like path? e.g. https://test.mock-bff.api.bbc.com/simorgh-bff?pageType=bob?
@@ -24,9 +24,6 @@ describe('Local Server', () => { | |||
${'Migrated Home Page'} | ${'/pidgin.json'} | ${'/pidgin/homePage/index.json'} | |||
${'Home Page'} | ${'/kyrgyz/tipohome.json'} | ${'/kyrgyz/homePage/index.json'} | |||
${'Most Read'} | ${'/pidgin/mostread.json'} | ${'/pidgin/mostRead/index.json'} | |||
${'Most Read with variant'} | ${'/zhongwen/mostread/trad.json'} | ${'/zhongwen/mostRead/trad.json'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we mean to delete this?
${'Most Read with variant'} | ${'/zhongwen/mostread/trad.json'} | ${'/zhongwen/mostRead/trad.json'} | |
| ${'Most Read with variant'} | ${'/zhongwen/mostread/trad.json'} | ${'/zhongwen/mostRead/trad.json'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops no. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put back
…rgh-code' of github.com:bbc/simorgh into WSTEAMA-1157-decommission-most-watched-page-delete-simorgh-code
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
Helpful Links
Add Links to useful resources related to this PR if applicable.
Coding Standards
Repository use guidelines