-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat: Improvements to the toolbar launch page #28300
Conversation
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.
PR Summary
This PR enhances the toolbar launch page UX by improving error messaging, adding URL suggestions, and providing better guidance for users setting up authorized URLs.
- Added new
ToolbarLaunch.stories.tsx
with comprehensive test scenarios for different states (default, no URLs, no suggestions, empty) - Enhanced
AuthorizedUrlList
component with inline loading states and contextual help text in/frontend/src/lib/components/AuthorizedUrlList/AuthorizedUrlList.tsx
- Added conditional feature highlights for Web Experiments and Web Vitals based on feature flags in
/frontend/src/scenes/toolbar-launch/ToolbarLaunch.tsx
- Streamlined
authorizedUrlListLogic.ts
by removing unused query field and improving key generation logic
4 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
frontend/src/lib/components/AuthorizedUrlList/AuthorizedUrlList.tsx
Outdated
Show resolved
Hide resolved
frontend/src/lib/components/AuthorizedUrlList/AuthorizedUrlList.tsx
Outdated
Show resolved
Hide resolved
b854805
to
33f3d23
Compare
Size Change: +5 B (0%) Total Size: 1.17 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
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.
would have been cool to get the stories in and then the changes so we could see them 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.
Oh, absolutely. I'd totally do that if it wasn't for our flappy CI, hope you can understand :)
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.
🫠
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 rechecking is a great idea!
frontend/__snapshots__/scenes-other-toolbarlaunch--no-urls-template--dark.png
Outdated
Show resolved
Hide resolved
frontend/__snapshots__/scenes-other-toolbarlaunch--no-urls-template--light.png
Outdated
Show resolved
Hide resolved
} | ||
|
||
export const authorizedUrlListLogic = kea<authorizedUrlListLogicType>([ | ||
path((key) => ['lib', 'components', 'AuthorizedUrlList', 'authorizedUrlListLogic', key]), | ||
key((props) => (props.experimentId ? `${props.type}-${props.experimentId}` : `${props.type}-${props.actionId}`)), | ||
key((props) => | ||
props.experimentId |
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 know you didn't add this but we could just template all these things into the string regardless of presence... it doesn't matter if we ahve undefined
in the key so long as its stable and it is one less thing to think about
total nitpick though
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.
We probably should do this as the values for experimentId
and actionId
can overlap. Unlikely to be a problem in practice but things might change 🤷
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.
That's an excellent point, thanks guys :)
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.
only scanned the code. functionality looks good to me
dumped a few nits around the place but you're welcome to ignore or follow-up instead of looping here
💯 for adding the stories
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.
Did a quick scan, I agree with @pauldambraand don't have anything else to add :)
389b00c
to
f1b5dd1
Compare
This might make our features slightly more discoverable
There are cases where neither action or experiment are passed id, so let's create that store properly
`query` is not being used inside the logic, so let's remove it
The toolbar authorized url list empty state is now much nicer. We're giving people more explanation on what's happening and what they should do to get the toolbar to work. We're also allowing them to refresh the suggestion to grab domains from new events.
f1b5dd1
to
8568648
Compare
📸 UI snapshots have been updated10 snapshot changes in total. 0 added, 10 modified, 0 deleted:
Triggered by this commit. |
Problem
This page is not our best, some customers are confused about how they should use it.
Solution
Let's add much more meaningful error messages and suggestions on what customers should do. Let's also allow them to easily refresh and get the newly suggested URLs.