Skip to content
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): UX refresh for the no-code toolbar. #25190

Open
wants to merge 53 commits into
base: master
Choose a base branch
from

Conversation

Phanatic
Copy link
Contributor

@Phanatic Phanatic commented Sep 24, 2024

Problem

@corywatilo and I did a design review of the no-code toolbar and found a lot of issues we could fix easily.
Below is the list:

  1. bug: h2, span is not selectable.
  2. bug: Cannot enter experiment name.
  3. ux: Control shouldn't ever change anything
  4. ux: Control shouldn't need to add any transforms.
  5. ux: Should not delete control
  6. ux: No visual frame to tell which variant, maybe create accordions.
  7. feat: should be able to edit the selector.
  8. feat: element selector should always
  9. Remove should be just the trash icon
  10. visualize section whenever its expanded.
  11. Control should always be first.
  12. Placeholder on accordion title.
  13. Move all controls to the header.
  14. Control blankslate
  15. Open test variant when creating a new experiment.

Changes

Before

nct_ux_stale

After

nct_ux_refresh

After

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

Manually, in local development

@Phanatic Phanatic changed the title feat(experiments) : UX refresh for the no-code toolbar. feat(experiments): UX refresh for the no-code toolbar. Sep 24, 2024
Copy link
Contributor

github-actions bot commented Sep 24, 2024

Size Change: -939 kB (-45.51%) 🎉

Total Size: 1.12 MB

Filename Size Change
frontend/dist/toolbar.js 1.12 MB -939 kB (-45.51%) 🎉

compressed-size-action

@jurajmajerik
Copy link
Contributor

Do we need the Cancel/Save buttons here? I think a simple text input would do.

image

@jurajmajerik
Copy link
Contributor

Clicking on the accordion items doesn't expand them.

Screen.Recording.2024-09-25.at.16.07.36.mov

@jurajmajerik
Copy link
Contributor

jurajmajerik commented Sep 25, 2024

I'm not sure I like having the select button directly on the accordion header.

image

The purpose of the accordion header is only to collapse/uncollapse an accordion item. I don't think it's a good idea to mix it with the functionality to select an element.

There is no point selecting an element if a particular accordion item is collapsed, so in that case that functionality should be hidden.

My suggestion:

  • Put the "Select element" button inside the accordion item content
  • In the accordion header, render either:
    • The element selector string (make sure it's truncated, these can get pretty long and break the UI)
    • Placeholder if no element selected: No element selected or similar

@jurajmajerik
Copy link
Contributor

Change text to Select element

No need to be courteous on buttons :)) it should convey the meaning and take up as little space as possible

image

@jurajmajerik
Copy link
Contributor

I would just use a ➕ icon here. The target icon is already used by the "Select element" button. Don't repeat icons for different buttons (generally)

image

@jurajmajerik
Copy link
Contributor

image

We have a standardized success tag for the dark mode (LemonTag type="success") which looks like this:

image

@jurajmajerik
Copy link
Contributor

image

Capitalize first letter: Please enter ...

@jurajmajerik
Copy link
Contributor

jurajmajerik commented Sep 25, 2024

I'd suggest a bit more elaborate copy here:

image

You're viewing the control variant, which represents your page in its original state.

@jurajmajerik
Copy link
Contributor

Do not allow removing all elements for a variant. At least one must always be present. Otherwise it's the same as control

image

@jurajmajerik
Copy link
Contributor

Do not allow removing the last test variant - we always need the control and at least 1 test variant

image

@jurajmajerik
Copy link
Contributor

Variant keys should be editable. Again, I suggesting putting the input field inside the accordion content, and only render the variant key on the accordion header (similar to #25190 (comment))

image

@jurajmajerik
Copy link
Contributor

The main action button Create experiment should be positioned to the right, and Cancel to the left

image

@jurajmajerik
Copy link
Contributor

There's a horizontal overflow here, rendering a scroll bar

Screen.Recording.2024-09-25.at.16.34.42.mov

}
return 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol thank you for doing this

@@ -3,8 +3,10 @@ import { cssEscape } from 'lib/utils/cssEscape'
import { ElementType } from '~/types'

// these plus any element with cursor:pointer will be click targets
export const CLICK_TARGETS = ['a', 'button', 'input', 'select', 'textarea', 'label', 'h1']
export const CLICK_TARGETS = ['a', 'button', 'input', 'select', 'textarea']
export const EXPERIMENT_TARGETS = ['label', 'h1', 'h2', 'h3', 'h4', 'span', 'img']
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jurajmajerik or @corywatilo is this a good set of elements to run experiments on? I don't want to put in a * here and would rather limit the set.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

4 snapshot changes in total. 0 added, 4 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants