-
Notifications
You must be signed in to change notification settings - Fork 6
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
[AXON-41] Add error boundary to all pages except JIRA #59
Conversation
99ea619
to
6d24736
Compare
SettingsPage = 'settings', | ||
WelcomePage = 'welcome', | ||
|
||
BitbucketIssuePage = 'bitbucketIssue', |
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.
Are we happy with the naming conventions here as we look at the next several years?
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.
The honest answer is that I'm not particularly happy with any of this, lol :D The idea was to somehow list the pages we have today, to somehow represent them in analytics. This isn't very future proof - for what it's worth, it's kind of already not very "present-proof", with how there are 2 start work
pages xD
Super open to alternative suggestions here
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.
Here are some prior art on this:
https://posthog.com/docs/product-analytics/best-practices
In summary:
- use colon to separate main ideas
:
- use underscore to separate words
_
in a given idea - start with the high level and go to the low level
- version the appropriate idea (eg: v1, v2, v3 ...)
- do keep a consistent vocabulary (eg: always
...button:click
, never...clicky_thing:tap
)
Good Example:
start_work_page:start_work_button:click
start_work_page_v2:start_work_button:click
Don'ts:
- Don't use old vs new. What happens when you need the next news?
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.
Hmm, I'm happy with any naming convention here, so let's go with this :) Let's address it in a follow-up PR though if that's OK? Just to minimize the number of rebases :D
@@ -138,6 +140,7 @@ export function usePipelineSummaryController(): [PipelineSummaryState, PipelineS | |||
refresh: sendRefresh, | |||
rerun: rerun, | |||
fetchLogs: fetchLogs, | |||
postMessage: postMessage, | |||
}; | |||
}, [sendRefresh, rerun, fetchLogs]); |
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.
See GitHub Action warning about React Hook
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.
Couple of questions
@@ -8,6 +8,24 @@ export enum AnalyticsChannels { | |||
AtlascodeUiErrors = 'atlascode.ui.errors', | |||
} | |||
|
|||
export enum AnalyticsView { | |||
OnboardingPage = 'onboarding', |
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.
Example here:
onboarding_page:view
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.
Approving with the note that we will have a follow up for renaming the analytics events
The base branch was changed.
6d24736
to
a289c70
Compare
Rebased onto main, merging |
What is this?
AtlascodeErrorBoundary
is now added to all pages of the newer architectureHow was this tested?