-
Notifications
You must be signed in to change notification settings - Fork 4
feat: add a step to the stepper for academy selection and update lang… #128
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #128 +/- ##
==========================================
+ Coverage 84.12% 85.20% +1.08%
==========================================
Files 147 149 +2
Lines 2444 2528 +84
Branches 410 482 +72
==========================================
+ Hits 2056 2154 +98
+ Misses 388 374 -14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
brobro10000
left a comment
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.
When creating the essentials feature, please keep in mind the teams feature is a complete feature and the original application behavior should not be interrupted as its release imminent.
Hide the essentials logic behind the feature flag and ensure the stepper remains a 3 step component for now until eseentials is ready to release. Please review the product requirements of the essentials page bearing in mind the following:
- The essentials page is both an authenticated and unauthenticated route
- The essentials page initial point of entry is from business.edx.org (external) so the stepper upcoming stepper (plan-details etc.) behavior is completely dependent on whether a user selects an academy or chooses to upgrade
- The teams page initial entry point is
/plan-detailsand the original logic should remain that if the user directly navigates to plan details with an incomplete checkout intent object the stepper flow is dependent on the product the user select (either a teams or essentials product).
I am available to pair to help get this over the line and make the requirements clear.
| authenticatedUser: { userId: 12345 }, | ||
| }); | ||
| validateText('Continue'); | ||
| expect(screen.getByText('Continue')).toBeInTheDocument(); |
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.
This can remain as validateText since it does essentially the same thing.
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.
yes
| expect( | ||
| screen.getByText('What is the name of your company or organization?'), | ||
| ).toBeInTheDocument(); |
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.
This can remain as validateText since it does essentially the same thing.
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.
yes
| expect( | ||
| screen.getByText('Create a custom URL for your team'), | ||
| ).toBeInTheDocument(); | ||
| }); |
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.
This can remain as validateText since it does essentially the same thing.
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.
yes
|
|
||
| fireEvent.click(screen.getByText('Back')); | ||
|
|
||
| // expect(screen.queryByText('Plan Details')).toBeInTheDocument(); |
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.
Is this supposed to be commented out?
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.
yes
| if (authenticatedUser) { | ||
| // If the user is already authenticated, redirect to AcademicSelection Page. | ||
| return redirect(CheckoutPageRoute.AcademicSelection); |
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.
I would retain the original logic to PlanDetails since AcademicSelection should only be navigable via business.edx.org
|
|
||
| const Steps = (): ReactElement => ( | ||
| <> | ||
| <AcademicSelection /> |
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.
Hide behind feature flag since it is an incomplete feature. See rootloader.ts.
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.
Remove this file
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.
Done
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.
Remove this file
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.
Done
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.
Remove this file
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.
Done
| const { step, substep } = params; | ||
|
|
||
| // Essentials uses substep as the step | ||
| const resolvedStep = step ?? substep ?? CheckoutStepKey.AcademicSelection; | ||
|
|
||
| const currentStepKey = resolvedStep as CheckoutStepKey; | ||
|
|
||
| const currentSubstepKey = step && substep ? substep : undefined; | ||
|
|
||
| const currentStep = CheckoutStepByKey[currentStepKey]; | ||
| const currentSubstep = currentSubstepKey | ||
| ? CheckoutSubstepByKey[currentSubstepKey] | ||
| : undefined; |
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.
I believe the original implementation was correct.
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.
yes
|
This POC should help guide you in the right direction for how to safely integrate academic selection into the current stepper process while it being gated by a feature flag. |
|
This is no longer needed, as the academy selection page is being removed from the stepper flow. As noted by Alberto Del Toro and with Hamzah creating a new ticket under the new architecture, so the PR is being closed. |
…uage
Added an “Academy Selection” step to the stepper, including initial setup, updated language, and i18n support; this step is hidden for users who enter directly into the Teams flow.

Added the Academy Selection stepper to the essentials/academic-selection route.