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

Frosted promos + isite #9732

Merged
merged 5 commits into from
Dec 9, 2021
Merged

Frosted promos + isite #9732

merged 5 commits into from
Dec 9, 2021

Conversation

ryanmccombe
Copy link
Contributor

@ryanmccombe ryanmccombe commented Dec 9, 2021

Resolves [No Issue]

Overall change:
Allows promos to be controlled by toggles.

Code changes:

  • A bullet point list of key code changes that have been made.
  • When describing code changes, try to communicate how and why you implemented something a specific way, not just what has changed.

  • I have assigned myself to this PR and the corresponding issues
  • I have added the cross-team label to this PR if it requires visibility across World Service teams
  • I have assigned this PR to the Simorgh project
  • (BBC contributors only) This PR follows the repository use guidelines

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 yarn test:e2e:interactive)
  • This PR requires manual testing

@ryanmccombe ryanmccombe self-assigned this Dec 9, 2021
@ryanmccombe ryanmccombe added this to Code Review in Simorgh Dec 9, 2021
Comment on lines +4 to +10
export default loadable(
() =>
import(
/* webpackChunkName: "frosted_promo" */
'.'
),
{ fallback: <span data-testid="frosted-promo-loader" /> },
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks cool. So the browser doesn't load the component chunk outlined until it comes into view?

Copy link
Contributor Author

@ryanmccombe ryanmccombe Dec 9, 2021

Choose a reason for hiding this comment

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

Ah I'm not sure Loadable cares about scroll position - but it does exclude the code from the bundle if the component is not actually invoked at runtime though.

eg, without loadable, this component and it's subtree (which is quite big because of the colour picker) would need included in the bundle, even if the component was toggled off on isite

src/app/containers/CpsFeaturesAnalysis/index.jsx Outdated Show resolved Hide resolved
src/app/containers/CpsFeaturesAnalysis/index.jsx Outdated Show resolved Hide resolved
Copy link
Contributor

@HarveyPeachey HarveyPeachey left a comment

Choose a reason for hiding this comment

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

LGTM Just some added some suggestions

@ryanmccombe ryanmccombe merged commit e1fc7dd into latest Dec 9, 2021
@ryanmccombe ryanmccombe deleted the frosted-promos-isite branch December 9, 2021 21:42
@ryanmccombe ryanmccombe moved this from Code Review to Done in Simorgh Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants