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.

Sign up for GitHub

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

Jump to bottom

Remove FrontPageMain and move logic to /pages #5662

Merged
merged 15 commits into from
Feb 27, 2020
Merged

Remove FrontPageMain and move logic to /pages #5662

merged 15 commits into from
Feb 27, 2020

Conversation

DenisHdz
Copy link
Contributor

@DenisHdz DenisHdz commented Feb 24, 2020 •
edited
Loading

Resolves #5169

Overall change:
Move the logic and tests from the FrontPageMain container to the FrontPage page.

Code changes:

  • Update /pages/FrontPage
  • Update FrontPage stories
  • Update FrontPage tests
  • Remove FrontPageMain container
  • I have assigned myself to this PR and the corresponding issues
  • I have added labels to this PR for the relevant pod(s) affected by these changes
  • I have assigned this PR to the Simorgh project

Testing:

  • Automated (jest and/or cypress) tests added (for new features) or updated (for existing features)
  • If necessary, I have run the local E2E non-smoke tests relevant to my changes (CYPRESS_APP_ENV=local CYPRESS_SMOKE=false npm run test:e2e:interactive)
  • This PR requires manual testing

@DenisHdz DenisHdz self-assigned this Feb 24, 2020
@sareh sareh added the blocked This issue should not be worked on until another internal issue is completed - see desc for details label Feb 24, 2020
@sareh
Copy link
Contributor

sareh commented Feb 24, 2020

Blocking this on the Desktop layout work being merged, since this will cause conflicts and we want to minimise issues since Desktop should be merged tomorrow, pending going live on Wednesday.

thekp
thekp approved these changes Feb 25, 2020
View reviewed changes
Copy link
Contributor

@thekp thekp left a comment

Choose a reason for hiding this comment

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

Looks good, the forecast says conflicts are coming. ⛈

src/app/pages/FrontPage/index.test.jsx Outdated Show resolved Hide resolved
@thekp thekp self-requested a review February 25, 2020 09:25
@DenisHdz DenisHdz removed the blocked This issue should not be worked on until another internal issue is completed - see desc for details label Feb 25, 2020
EinsteinNjoroge
EinsteinNjoroge approved these changes Feb 26, 2020
View reviewed changes
Copy link
Contributor

@EinsteinNjoroge EinsteinNjoroge left a comment

Choose a reason for hiding this comment

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

Looks good

@PriyaKR PriyaKR self-assigned this Feb 26, 2020
thekp
thekp approved these changes Feb 26, 2020
View reviewed changes
Copy link
Contributor

@thekp thekp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for resolving the conflicts 💪

@PriyaKR
Copy link
Contributor

PriyaKR commented Feb 27, 2020

Looks good to me.Did a general regression across articles,MAPs,PGL and frontpages.

@DenisHdz DenisHdz merged commit bd7f78d into latest Feb 27, 2020
@DenisHdz DenisHdz deleted the split-frontpagemain branch February 27, 2020 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thekp thekp thekp approved these changes

@EinsteinNjoroge EinsteinNjoroge EinsteinNjoroge approved these changes

Assignees

@PriyaKR PriyaKR

@DenisHdz DenisHdz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove FrontPageMain, and move logic to /pages
5 participants
@DenisHdz @sareh @PriyaKR @EinsteinNjoroge @thekp