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

Remove FrontPageMain, and move logic to /pages #5169

Closed
1 task
tochwill opened this issue Jan 16, 2020 · 1 comment · Fixed by #5662
Closed
1 task

Remove FrontPageMain, and move logic to /pages #5169

tochwill opened this issue Jan 16, 2020 · 1 comment · Fixed by #5662
Assignees

Comments

@tochwill
Copy link
Contributor

tochwill commented Jan 16, 2020

Is your feature request related to a problem? Please describe.
We are removing the 'Main' containers across the board, and splitting up our page level containers into one 'page' per page type. You can see an example of the work that was done for CPS Asset pages here: #5166. This is the first step in a broader set of changes to avoid branching based on page type within containers, make it clearer what the concept of a 'container' is in Simorgh, and to move processing out of our components.

We have moved all of the Page containers into a new directory, /pages. As well as the Page containers, we also currently have a ‘Main’ container for each page type, which are still in the containers folder. For example, Article and ArticleMain, FrontPage and FrontPageMain, RadioPage and RadioPageMain. We also had one container for a CPS asset, as well as its corresponding ‘Main’ container. When using this approach, the Page container just wrapped the ‘Main’ container with the higher order components that were being used to enhance each page.

We have now decided that we should have a Page container for each CPS asset type, such as CpsPgl, CpsSty, and CpsMap. This is because the pages are different enough that we would have needed lots of branching logic within a generic CPS container, and it would have got messy and difficult to reason about. So, in the /pages directory, we have created these.

We also decided that it wasn’t really useful to have the container in the /pages directory that didn’t do anything other than wrap a 'Main' container with the higher order components - each of these containers was doing exactly the same thing. So we decided to move all of the logic from the 'Main' container into its corresponding top level container in the /pages directory, and delete the 'Main' containers. Now, for each page type, you can find the container for that page type in the /pages directory, and see all of the logic for that page type in one place. If we introduce a new page type, we add a container for it in the /pages directory.

We have already used this approach for the CpsPgl, CpsSty, and CpsMap pages, and we have issues raised to do this for all of the other page types. This is step one of a few tasks to clean up our containers, make the distinction between a ‘component’ and a ‘container’ clearer, and rethink how we process data in Simorgh to move it away from various levels of the component tree (more info is here: #3962)

Describe the solution you'd like
Move the logic and tests from the FrontPageMain container to the FrontPage page, following the approach of the pull request referenced above.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Testing notes
[Tester to complete]

Dev insight: Will Cypress tests be required or are unit tests sufficient? Will there be any potential regression? etc

  • This feature is expected to need manual testing.

Additional context
Add any other context or screenshots about the feature request here.

@tochwill tochwill added Refinement Needed This is ready for refinement. It shouldn't be worked on until it has been refined by Dev & Test. ws-home labels Jan 16, 2020
@tochwill
Copy link
Contributor Author

@tochwill to clear this up in written form before refining

@DenisHdz DenisHdz self-assigned this Feb 24, 2020
@DenisHdz DenisHdz removed the Refinement Needed This is ready for refinement. It shouldn't be worked on until it has been refined by Dev & Test. label Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants