-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Redesign splash page #20155
Redesign splash page #20155
Conversation
Hi @AkashPaloju please assign the required reviewer(s) for this PR. Thanks! |
I will assign the reviewers once all tests are passed. Thanks! |
Current Status: Waiting for reply on this query before the 'a11y' commit |
@AkashPaloju I've replied to the query. Are you planning to finish this PR? Please also note that your CI checks are failing. |
Also, one note -- where possible, try to optimize the image sizes, especially for the larger files. Thanks! |
Thanks @AkashPaloju, I am also adding the design leads for a review of the video! |
Hi, Web version:
Mobile version:
Tablet version:
Thank you for your great work! |
Hi Team, For Web: Kurin has done a great job capturing most of the changes needed, especially the mobile CTA alignments issues. No other issues on my end. Nice work all! |
Unassigning @nikitaevg since they have already approved the PR. |
Hi @AkashPaloju, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks! |
The time stamps here are for the first video on this page: Learn anything, anywhere with Oppia—your free educational platform. At 0:07 — having your *own account. And add a line break before Through our Learner Groups. 0:10: Oppia is a non-profit project built by volunteers from around the world. We always need more people to create, improve and translate lessons, contribute graphics, and more! Also only capitalize Subscribe in Subscribe to our newsletter at the bottom. Since we are moving away from title case, don’t capitalize Teachers or Parents in the header for “For Teachers” and “For Parents” Also, there’s not consistency with these buttons. Are we doing title case in buttons? I thought we weren’t, but I’m unsure. @seanlip |
Hi @S4v8n , |
Hi Akash, Thank you for the latest changes. Here are my comments. These are smaller things I noticed that more spacing related. Web:
Mobile Tablet Thank you again for your work, it is looking really good. Best, |
@S4v8n , I have addressed your comments and verified things you said, PTAL at the updated video. Thanks! |
@AkashPaloju looks good! Thanks for the quick turn around. I have no further comments. |
@seanlip Reviewers and Designers have approved the changes and all tests are passed. Please add this to Merge queue. Thanks! |
Hi @AkashPaloju, sorry for the last minute notes, but since this is going to get launched once it's merged I thought I'd better do a quick pass through the text. In some cases we are promising things that are not live yet. Could you please make the following changes and file an issue to revert them once the relevant functionality for learner groups and multiple classrooms is in place?
Also, some general textual updates (that can be permanent):
Also @stepmalt, re consistency of button text, that's up to the UXW team. Could you please confirm what the current style guide recommendation is? Finally @AkashPaloju Can you put these changes behind a feature flag so we can do A/B experiments on the splash pages? I think that would help us gain confidence in the new page before fully launching it. Thanks! (And sorry for the late response.) |
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.
Thanks @AkashPaloju for the PR, sorry for my late reply on it.
I was thinking more about the launch for this and I realized that it would probably be good to ensure that the design actually works well, so I suggest that we do a 50% feature rollout for this since we now have the functionality to do so.
(Note that for the other static pages you're doing though, I think we can definitely just go ahead with those changes since the current pages have clearer deficiencies. But for the splash page I think it is worth doing this since we still have traffic from the current page, and also this -- i.e. A/B tests -- is the sort of thing we should probably get some experience doing. PTAL at the changes suggested and let me know if you think that any of them would be difficult to do, I know the system reasonably well and can probably advise. Thanks!)
@@ -137,6 +137,30 @@ export class SiteAnalyticsService { | |||
); | |||
} | |||
|
|||
registerClickCreateAccountButtonEvent(): void { | |||
this._sendEventToLegacyGoogleAnalytics( |
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.
@AkashPaloju I don't think legacy GA works any more, we should remove it. Let's use the non-legacy version of this instead.
Also could you please add another parameter along with the events common to both pages that allows us to pass along the value of the feature flag? Then we can roll this out at 50% and see how the click rates compare between the two.
[innerHTML]="'I18N_SPLASH_ICON_ONE_TEXT' | translate"> | ||
</span> | ||
</div> | ||
<div class="oppia-splash-features-section" dir="auto" role="region" > |
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.
Hi @AkashPaloju -- would it be possible to make a splash-page-one.component.html and a splash-page-two.component.html, then use a feature flag value to ng-if between those? That way we can get some validation of this page before we launch it fully, so that we can make sure it is better than the current one.
We can keep that running for a couple months and then analyze the results at the end to decide which version to go with finally. OK for both these pages to use the same JS component if that makes things easier, and OK for these pages to contain duplicate code if needed since we're going to drop one anyway.
Hi, @AkashPaloju, the LGTM Label has been removed because the changes were requested on this PR. Thanks!. |
I understood that the primary thing we need to do is (pls tell me if I am wrong) :
Thanks! |
Yes, that's all 100% correct. Thanks for checking! |
Hi @AkashPaloju, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
Overview
Essential Checklist
Please follow the instructions for making a code change.
Proof that changes are correct
Desktop & Mobile devices
splash-revisions.webm
Hover effects
splash-hover-effects.webm
Lighthouse tests in a11y
Screencast.from.23-04-24.10.45.31.PM.IST.webm
RTL Layout:
Screencast from 23-04-24 10:43:28 PM IST.webm
Slow Network:
Screencast.from.23-04-24.10.51.37.PM.IST.webm
PR Pointers