feat: Add appless pre-PAT onboarding container #3629
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## cy/non_pat_appless #3629 +/- ##
=====================================================
Coverage ? 98.78%
=====================================================
Files ? 824
Lines ? 14794
Branches ? 4205
=====================================================
Hits ? 14614
Misses ? 171
Partials ? 9
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## cy/non_pat_appless #3629 +/- ##
=====================================================
Coverage ? 98.78%
=====================================================
Files ? 824
Lines ? 14794
Branches ? 4197
=====================================================
Hits ? 14614
Misses ? 171
Partials ? 9
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## cy/non_pat_appless #3629 +/- ##
=====================================================
Coverage ? 98.78%
=====================================================
Files ? 824
Lines ? 14794
Branches ? 4197
=====================================================
Hits ? 14614
Misses ? 171
Partials ? 9
Continue to review full report in Codecov by Sentry.
|
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Bundle ReportChanges will increase total bundle size by 6.27MB (100.0%) ⬆️
|
Bundle ReportChanges will increase total bundle size by 64.89kB (0.52%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
There was a problem hiding this comment.
I'm not really following why we need React context and localstorage. Could you explain how they work together?
There was a problem hiding this comment.
I implemented with React context and localStorage to together solve for two situations.
- Persistence over time: I'm using localStorage to keep track between sessions and refreshes. So if a user hid it previously, it will stay hidden
- Toggling show vs hide in one session: using just the localStorage value to allow "showing" and "hiding" from the UserDropdown menu wasn't working because the react tree wasn't rerendering on just a localStorage value change. So I decided to use a setState variable. However, the onboarding container and the UserDropdown components are part of two different component trees so I added a context to share the state variable. The only shared parent they have is App.tsx and I decided against implementing a setState variable in App.tsx to be passed to each of them as I didn't want to clutter up App.
There was a problem hiding this comment.
Interestinggggg okay. That's annoying but makes sense. In that case, one improvement I think we can do is handle all the local storage stuff inside of the context provider through a useEffect on the state change. That way the only place we interact with local storage is in the provider and the consumers don't have to worry about it.
I was also wondering why the need for the query param? Can't we use local storage/context for that as well?
There was a problem hiding this comment.
Hmm, you're right. I'll move the extra outside instance of localStorage into the context.
For the query param, I was piggybacking off of our current use of ?source=onboarding but not that I think about it more, we'll actually need to modify that construct too as we will no longer be showing DefaultOrgSelector anywhere in the flow. I will:
- make a change for the onboarding container to rely on localStorage
- make a change in useUserAccessGate to remove the showDefaultOrgSelector logic
- make a change in the onboarding flow to set the source param to "onboarding" again as a replacement for ListRepo's use
I considered using the same construct for both bullet 1 and 3, but the localStorage value is tied more closely to whether or not we render the onboarding container while the query param is tied to the idea we came from onboarding
There was a problem hiding this comment.
Gotcha makes sense. Could be useful to have that query param for metrics in the future.
calvin-codecov
force-pushed
the
cy/how_to_onboard
branch
from
af03d1b to
df3cd9f
Compare
| /> | ||
| </Route> | ||
| </Switch> | ||
| <OnboardingContainerProvider> |
There was a problem hiding this comment.
only change here is wrapping in the Provider component
| const url = new URL(window.location.href) | ||
| url.searchParams.set('source', ONBOARDING_SOURCE) | ||
| window.location.href = url.toString() |
There was a problem hiding this comment.
adding a query param that used to be applied after the DefaultOrgSelector screen
There was a problem hiding this comment.
Do we need both of these images?
| @@ -112,15 +97,11 @@ function BaseLayout({ children }: React.PropsWithChildren) { | |||
| <Suspense> | |||
| <ErrorBoundary errorComponent={<EmptyErrorComponent />}> | |||
| <SilentNetworkErrorWrapper> | |||
| {isFullExperience || isImpersonating ? ( | |||
| {(isFullExperience || isImpersonating) && ( | |||
There was a problem hiding this comment.
In general I think we prefer to use ternaries over && for conditional rendering. Ask Nick if curious there's some reason I can't remember lol.
There was a problem hiding this comment.
you are correct. Lemme change this and the other instance
| } | ||
|
|
||
| function OnboardingOrChildren({ | ||
| children, | ||
| isImpersonating, |
There was a problem hiding this comment.
I believe the intention here is to never show onboarding when impersonating. What's the status of this for the new flow? It might not be relevant anymore, but I'm not sure. You could ask AJ to confirm whether this is something we want.
src/pages/OwnerPage/OwnerPage.jsx
Outdated
| @@ -74,6 +78,7 @@ function OwnerPage() { | |||
| </SilentNetworkErrorWrapper> | |||
| </Suspense> | |||
| <div> | |||
| {showOnboardingContainer && <OnboardingOrg />} | |||
There was a problem hiding this comment.
Same thing re: ternaries here
calvin-codecov
force-pushed
the
cy/non_pat_appless
branch
from
4a1350e to
2e32017
Compare
…al, remove customer intent
calvin-codecov
force-pushed
the
cy/how_to_onboard
branch
from
e5e1888 to
b18e4a4
Compare
Description
Closes codecov/engineering-team#2735 and codecov/engineering-team#2740
Add
?source=onboardingto the end of the org page url and reload. This will trigger the onboarding container to show upNotable Changes
Figma file: https://www.figma.com/design/SsoxtY2SB73l0wiLbJ8FhZ/GH-2092?node-id=2288-16704&t=Z9J60jqfEmR3QjIK-1
Screenshots
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.