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.91%
=====================================================
Files ? 813
Lines ? 14508
Branches ? 4105
=====================================================
Hits ? 14351
Misses ? 150
Partials ? 7
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.91%
=====================================================
Files ? 813
Lines ? 14508
Branches ? 4112
=====================================================
Hits ? 14351
Misses ? 150
Partials ? 7
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.91%
=====================================================
Files ? 813
Lines ? 14508
Branches ? 4105
=====================================================
Hits ? 14351
Misses ? 150
Partials ? 7
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.91%
=====================================================
Files ? 813
Lines ? 14508
Branches ? 4112
=====================================================
Hits ? 14351
Misses ? 150
Partials ? 7
Continue to review full report in Codecov by Sentry.
|
Bundle ReportChanges will decrease total bundle size by 6.0MB (-32.96%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
Bundle ReportChanges will decrease total bundle size by 6.0MB (-32.96%) ⬇️. 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.
…al, remove customer intent
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
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.