Props Settings UI to match Goals Settings #3322

aerosol · 2023-09-05T08:50:12Z

Changes

This PR makes the Custom Props settings UI similar to the one introduced in #3293
~~The diff is against #3321 - please review/merge that one first, I'll then change the base branch.~~

There's several semi-related changes done by the way:

the funnels feature flag is completely removed now
live views preload external resources only once, at initial render, leveraging the shared assigns on the secondary render when the socket connects
in case of "creatable" ComboBox, the "Create ..." option shows on top but also the first suggestion is focused, if available

Tests

Automated tests have been added
This PR does not require tests

Changelog

Entry has been added to changelog
This PR does not make a user-facing change

Documentation

Docs have been updated
This change does not need a documentation update

Dark mode

The UI has been tested both in dark and light mode
This PR does not change the UI

bundlemon · 2023-09-05T12:48:44Z

BundleMon

Files updated (2)

Status	Path	Size	Limits
✅	static/js/app.js	40.29KB (+179B +0.44%)	-
✅	static/css/app.css	14.44KB (+75B +0.51%)	-

Unchanged files (5)

Status	Path	Size	Limits
✅	static/js/dashboard.js	318.6KB	-
✅	static/js/embed.host.js	5.58KB	-
✅	static/js/embed.content.js	5.08KB	-
✅	tracker/js/plausible.js	742B	-
✅	static/js/applyTheme.js	314B	-

Total files change +254B +0.06%

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

aerosol · 2023-09-06T07:53:27Z

Added c1e649b so in case of creatables and suggestions, the "Create ..." option is not focused by default, instead the first suggestion is highlighted.

ukutaht

Some UI feedback as well:

The trashcan icons are cut off somehow?

Hitting enter should work for the creatable option as well. For me it doesn't.
In the add property modal, when the dropdown is open, hitting escape should only close the dropdown. Currently it also hides the modal. To close dropdown and hide modal, one should hit escape twice.
The creatable option shows up very inconsistently for me:
https://github.com/plausible/analytics/assets/3731516/37da80e4-c159-4794-a3ac-c64f8266386f
The modal takes a few hundred ms to open. How about using async={true} for the Combobox to make the modal open interaction more snappy?

aerosol · 2023-09-07T12:02:43Z

The trashcan icons are cut off somehow?

and

Hitting enter should work for the creatable option as well. For me it doesn't.

Both on Safari? Can't repro on Firefox/Chrome, trashcans look fine and hitting enter on "Create..." closes the suggestion popup (unless this is not what you expect?).

aerosol · 2023-09-11T07:51:38Z

The modal takes a few hundred ms to open. How about using async={true} for the Combobox to make the modal open interaction more snappy?

As of ce1e2f6 the suggestions load asynchronously. I also addressed your previous remark about async being default everywhere, unless explicitly set to false. We no longer need to wait in tests either.

aerosol · 2023-09-11T11:01:04Z

The creatable option shows up very inconsistently for me:
https://github.com/plausible/analytics/assets/3731516/37da80e4-c159-4794-a3ac-c64f8266386f

96f314b should give much better experience

aerosol · 2023-09-11T11:04:35Z

In the add property modal, when the dropdown is open, hitting escape should only close the dropdown.

Surprisingly, this one is the hardest to do. On it...

aerosol · 2023-09-11T11:24:10Z

In the add property modal, when the dropdown is open, hitting escape should only close the dropdown.

I think what's going on is Firefox intercepts the escape on input[type=text] to blur focus.

I can't use x-on.blur event to close the dropdown because the blur is also fired when you click a suggestion with mouse, leading to a race condition where selecting with mouse is impossible then 😰

aerosol · 2023-09-12T07:08:31Z

The trashcan icons are cut off somehow?

Just tried Safari:

Can you reproduce @ukutaht ? 🤔

ukutaht · 2023-09-12T13:16:59Z

@aerosol The creatable option focus state looks good now 👍

Hitting enter works fine on creatable. Honestly I think it was working the whole time and I was just confused last time I reviewed...

Trashcan icons are still cut off on MacOS Safari but not on MacOS Firefox. @zoldar I noticed you also have a Mac, could you help test if you see the same thing on this branch?

ukutaht · 2023-09-12T13:18:09Z

Sorry. Accidentally hit the wrong button ^

ukutaht · 2023-09-12T13:23:07Z

In the add property modal, when the dropdown is open, hitting escape should only close the dropdown.

Surprisingly, this one is the hardest to do. On it...

Don't have time to thoroughly test this right now but maybe something like this would work? b9e0205

aerosol · 2023-09-13T06:08:33Z

@ukutaht the problem I'm struggling with is intercepting the escape key while the input is focused, close() does not even get called. #3322 (comment)

x-on:keydown:escape on ComboBox input does nothing.

aerosol · 2023-09-13T06:09:20Z

@zoldar I noticed you also have a Mac, could you help test if you see the same thing on this branch?

I have access to a mac but can't reproduce still :/

This should be done only once in the live view

We require "Create ..." element to be only focused when there are no suggestions available. This causes some issues, depending on the state, the least focusable index might be either 0 ("Create...") or 1. This patch addresses all the quirks with focus.

So that AlpineJS doesn't think it's a focusable option.

aerosol · 2023-09-13T06:18:34Z

I thought it's maybe the tailwind upgrade, rebase against master, issued npm install --prefix assets, but still no luck. Deploying to staging.

aerosol · 2023-09-13T06:25:50Z

Staging rebased against master on Safari:

@ukutaht can you wipe your node_modules and try again perhaps?

Co-authored-by: Uku Taht <Uku.taht@gmail.com>

aerosol · 2023-09-13T07:56:22Z

7649166 works in the end, it was Vimium-C Firefox add-on blocking it... 🤦

@ukutaht

This reverts commit 306866d. @ukutaht unfortunately this makes it impossible to select an suggestion.

This reverts commit 4844857.

zoldar

I'll need to up my LV game but at least from what I can understand, it LGTM 😄 ✨ left a couple questions but no showstoppers

zoldar · 2023-09-13T11:32:48Z

lib/plausible_web/controllers/stats_controller.ex

+  defp get_flags(_user) do
+    %{}


Do we foresee using it again in the near future?

Yeah, for sure

zoldar · 2023-09-13T11:36:16Z

lib/plausible_web/live/components/combo_box.ex

-    <li :if={@idx == @suggestions_limit} class="text-xs text-gray-500 relative py-2 px-3">
-      Max results reached. Refine your search by typing in goal name.
-    </li>
+    <div :if={@idx == @suggestions_limit} class="text-xs text-gray-500 relative py-2 px-3">


Out of curiosity, why is it div instead of li? AFAIK it's still going to be embedded in a list so it's invalid (but parsable) HTML I think 🤔

Because it's later on easier to distinguish actual suggestions from non-suggestions in JS :D

zoldar · 2023-09-13T11:41:14Z

lib/plausible_web/live/funnel_settings.ex

+    socket =
+      socket
+      |> assign_new(:site, fn ->
+        Sites.get_for_user!(user_id, domain, [:owner, :admin, :super_admin])


I'm assuming that under normal circumstances, site_id and domain always match, unless somebody starts fiddling with arguments manually, right (additionally, from what I see, @site_id is not used anytwhere in LV templates for funnels anyway, only goals)?

Yeah. site_id and domain always match. Even if someone it able to inject that (and bypass server-side signed payload) there is nothing to achieve. And good, point I'll remove the site_id assign btw of #3323

zoldar · 2023-09-13T11:59:35Z

lib/plausible_web/live/props_settings.ex

+        filter_text: "",
+        all_props: [prop | socket.assigns.all_props],
+        displayed_props: [prop | socket.assigns.all_props],
+        site: %{site | allowed_event_props: [prop | site.allowed_event_props]}


That one is super hard for me to read - perhaps it would be better to split it into a temporary assignment of:

allowed_event_props = [prop | site.allowed_event_props]

and only then set it via:

... site: %{site | allowed_event_props: allowed_event_props} ...

wdyt?

sure! a4b3e00

aerosol force-pushed the props-settings-ui branch 3 times, most recently from 6ec8fd5 to aafc648 Compare September 5, 2023 09:02

aerosol marked this pull request as ready for review September 5, 2023 09:04

aerosol requested review from vinibrsl and ukutaht September 5, 2023 09:04

aerosol changed the title ~~Props settings UI~~ Props Settings UI to match Goals Settings Sep 5, 2023

Base automatically changed from capitalization to master September 5, 2023 12:43

vinibrsl force-pushed the props-settings-ui branch from 8a97405 to 6fa3508 Compare September 5, 2023 12:44

vinibrsl approved these changes Sep 5, 2023

View reviewed changes

aerosol force-pushed the props-settings-ui branch from 6fa3508 to c1e649b Compare September 6, 2023 07:52

aerosol mentioned this pull request Sep 6, 2023

Funnel Settings UI tweaks (to match Custom Props/Goals) #3323

Merged

8 tasks

ukutaht reviewed Sep 7, 2023

View reviewed changes

ukutaht closed this Sep 12, 2023

ukutaht reopened this Sep 12, 2023

aerosol added 2 commits September 13, 2023 08:14

Add hint to creatable ComboBoxes without suggestions available

9b41caa

Load external resources once in funnel settings

b7a3c09

aerosol added 12 commits September 13, 2023 08:14

Load external resources once in goal settings

b596d51

Make Custom Props Settings UI match Goal Settings

04393d1

Remove unnecessary goals query

8352dc6

This should be done only once in the live view

Remove funnels feature flag

8e8e9ab

fixup

caae672

Make the modal scrollable

17d3177

By default, focus first suggestion for creatables

54e2fb0

Add sample props to seeds

070acc5

Load all suggestions asynchronously, unless Mix.env == :test

838e5c0

Fix ComboBox max results message

6c4eb4b

So that AlpineJS doesn't think it's a focusable option.

Keep the state up to date when changing props

b22bdec

aerosol force-pushed the props-settings-ui branch from d8057bf to b22bdec Compare September 13, 2023 06:15

aerosol added the deploy-to-staging Special label that triggers a deploy to a staging environment label Sep 13, 2023

aerosol and others added 2 commits September 13, 2023 09:34

Update seeds with sensible prop names

8349ddc

Make escape work for closing combobox suggestions

306866d

Co-authored-by: Uku Taht <Uku.taht@gmail.com>

aerosol force-pushed the props-settings-ui branch from 7649166 to 306866d Compare September 13, 2023 07:56

aerosol added 3 commits September 13, 2023 12:52

Revert "Make escape work for closing combobox suggestions"

4844857

This reverts commit 306866d. @ukutaht unfortunately this makes it impossible to select an suggestion.

Revert "Revert "Make escape work for closing combobox suggestions""

7960637

This reverts commit 4844857.

Make ESC great again

0f42b57

zoldar approved these changes Sep 13, 2023

View reviewed changes

Improve readability

a4b3e00

aerosol merged commit 0822bc6 into master Sep 13, 2023
3 of 6 checks passed

aerosol deleted the props-settings-ui branch September 13, 2023 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Props Settings UI to match Goals Settings #3322

Props Settings UI to match Goals Settings #3322

aerosol commented Sep 5, 2023 •

edited

Loading

bundlemon bot commented Sep 5, 2023 •

edited

Loading

aerosol commented Sep 6, 2023

ukutaht left a comment •

edited

Loading

aerosol commented Sep 7, 2023

aerosol commented Sep 11, 2023

aerosol commented Sep 11, 2023

aerosol commented Sep 11, 2023

aerosol commented Sep 11, 2023

aerosol commented Sep 12, 2023

ukutaht commented Sep 12, 2023

ukutaht commented Sep 12, 2023

ukutaht commented Sep 12, 2023

aerosol commented Sep 13, 2023

aerosol commented Sep 13, 2023

aerosol commented Sep 13, 2023

aerosol commented Sep 13, 2023

aerosol commented Sep 13, 2023

zoldar left a comment

zoldar Sep 13, 2023

aerosol Sep 13, 2023

zoldar Sep 13, 2023

aerosol Sep 13, 2023

zoldar Sep 13, 2023

aerosol Sep 13, 2023

zoldar Sep 13, 2023

aerosol Sep 13, 2023

Props Settings UI to match Goals Settings #3322

Props Settings UI to match Goals Settings #3322

Conversation

aerosol commented Sep 5, 2023 • edited Loading

Changes

Tests

Changelog

Documentation

Dark mode

bundlemon bot commented Sep 5, 2023 • edited Loading

BundleMon

aerosol commented Sep 6, 2023

ukutaht left a comment • edited Loading

Choose a reason for hiding this comment

aerosol commented Sep 7, 2023

aerosol commented Sep 11, 2023

aerosol commented Sep 11, 2023

aerosol commented Sep 11, 2023

aerosol commented Sep 11, 2023

aerosol commented Sep 12, 2023

ukutaht commented Sep 12, 2023

ukutaht commented Sep 12, 2023

ukutaht commented Sep 12, 2023

aerosol commented Sep 13, 2023

aerosol commented Sep 13, 2023

aerosol commented Sep 13, 2023

aerosol commented Sep 13, 2023

aerosol commented Sep 13, 2023

zoldar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aerosol commented Sep 5, 2023 •

edited

Loading

bundlemon bot commented Sep 5, 2023 •

edited

Loading

ukutaht left a comment •

edited

Loading