-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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(experiments): add variant screenshots #25397
Conversation
Size Change: 0 B Total Size: 1.13 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated6 snapshot changes in total. 0 added, 6 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
…og/posthog into experiment-variant-preview
<div className="relative"> | ||
<div className="Lettermark Lettermark--xlarge Lettermark--rounded Lettermark--variant-4"> | ||
<IconUpload /> | ||
</div> | ||
</div> |
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.
Plugging into the internals of another component is fragile – changes to Lettermark styling in the future are likely to break VariantPreviewImage
. On a different note, there is very poor contrast between the upload icon and its background.
I propose a more conventional file input, which also lets us be more descriptive about this preview feature:
<div className="relative"> | |
<div className="Lettermark Lettermark--xlarge Lettermark--rounded Lettermark--variant-4"> | |
<IconUpload /> | |
</div> | |
</div> | |
<> | |
<IconUpload className="text-2xl" /> | |
<span>Upload a preview of this variant's UI</span> | |
</> |
As a bonus, this empty state also uses the available space much more efficiently than the XXL pseudo-lettermark.
<div className="text-muted inline-flex flow-row items-center gap-1 cursor-pointer"> | ||
<div onClick={() => setIsModalOpen(true)} className="cursor-pointer relative"> |
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 had no idea that I must click on this tile to see the preview in a large scale – only realized this looking at the code! In other file upload contexts you click on the image to change it (e.g. org logo or Hog destinations). But an easy way to create the affordance for image expansion is cursor: zoom-in
:
<> | ||
<div className="text-muted inline-flex flow-row items-center gap-1 cursor-pointer"> | ||
<div onClick={() => setIsModalOpen(true)} className="cursor-pointer relative"> | ||
<div className="relative flex overflow-hidden select-none size-16 rounded before:absolute before:inset-0 before:border before:rounded"> |
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.
Feedback addressed, thanks @Twixes! |
📸 UI snapshots have been updated8 snapshot changes in total. 0 added, 8 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.
👌
Changes
Users can now upload screenshots to visualize each variant in the Experiment UI.
Screen.Recording.2024-10-04.at.15.43.02.mov
How did you test this code?
👀