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

Add landing page experiment #5434

Merged
merged 36 commits into from
Jan 9, 2025
Merged

Add landing page experiment #5434

merged 36 commits into from
Jan 9, 2025

Conversation

flozia
Copy link
Collaborator

@flozia flozia commented Dec 19, 2024

References:

Jira:

Description

Adds the new view for the landing page experiment landing-page-redesign together with the feature flag LandingPageRedesign. The content of the page will be implemented together with MNTOR-3806.

How to test

Enable the feature flag LandingPageRedesign and visit /.

Checklist (Definition of Done)

  • Commits in this PR are minimal and have descriptive commit messages.
  • I've added or updated the relevant sections in readme and/or code comments
  • I've added a unit test to test for potential regressions of this bug.
  • If this PR implements a feature flag or experimentation, the Ship Behind Feature Flag status in Jira has been set
  • Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
  • All acceptance criteria are met.

Copy link

@flozia flozia self-assigned this Dec 19, 2024
@flozia flozia requested review from rhelmer and codemist December 20, 2024 15:42
Copy link
Collaborator

@rhelmer rhelmer left a comment

Choose a reason for hiding this comment

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

@flozia code changes look OK, I tried enabling locally and all I see is this on /. I noticed it's not passing tests too is this ready for review?
image

@@ -101,6 +101,33 @@ features:
value: { "enabled": true }
- channel: production
value: { "enabled": true }
landing-page-redesign:
description: Landing page redesign
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this might be confusing later, could we give it a more unique name? Do we ever remove these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point — I’ve updated the experiment ID to be a bit more explicit in bd0974e.

@flozia
Copy link
Collaborator Author

flozia commented Jan 8, 2025

@flozia code changes look OK, I tried enabling locally and all I see is this on /. I noticed it's not passing tests too is this ready for review?

@rhelmer This PR is only adding the new landing page redesign page together with the MobileShell updates. The rest of the page is being implemented together with MNTOR-3806. It looks like the unit test failure is the same coverage issue we’ve experienced back in December — looking into it.

Comment on lines +83 to +98
<LandingViewRedesign
eligibleForPremium={eligibleForPremium}
l10n={getL10n()}
countryCode={countryCode}
scanLimitReached={scanLimitReached}
experimentData={experimentData["Features"]}
/>
) : (
<LandingView
eligibleForPremium={eligibleForPremium}
l10n={getL10n()}
countryCode={countryCode}
scanLimitReached={scanLimitReached}
experimentData={experimentData["Features"]}
/>
)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these two meant to be the same?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One is the new LandingViewRedesign layout and the other is the current LandingView — both are taking the same props.

Copy link
Collaborator

@codemist codemist left a comment

Choose a reason for hiding this comment

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

LGTM! I had one comment.

@flozia flozia merged commit 8b97f7c into main Jan 9, 2025
16 checks passed
@flozia flozia deleted the mntor-3825 branch January 9, 2025 20:00
Copy link

github-actions bot commented Jan 9, 2025

Cleanup completed - database 'blurts-server-pr-5434' destroyed, cloud run service 'blurts-server-pr-5434' destroyed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants