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

Toolbar Heatmap & Inspect improvement #1424

Merged
merged 10 commits into from
Aug 18, 2020
Merged

Toolbar Heatmap & Inspect improvement #1424

merged 10 commits into from
Aug 18, 2020

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented Aug 13, 2020

Changes

  • This PR fixes a bunch of tiny issues with element selection
  • It also upgrades Simmer to a version that uses [data-attr] in its generated selectors when available.

Before:
image
After:
image


Before:
image
After:
image


Before:
image
After:
image


Before:
image
After:
image


Before:
image
image
After:
image
image

Checklist

  • All querysets/queries filter by Team (if this PR affects any querysets/queries)
  • Backend tests (if this PR affects the backend)
  • Cypress E2E tests (if this PR affects the front and/or backend)

@timgl timgl temporarily deployed to posthog-toolbar-element-ie4k12 August 13, 2020 14:21 Inactive
@mariusandra mariusandra changed the title WIP - toolbar element selection improvement [WIP] toolbar element selection improvement Aug 14, 2020
…), upgrade simmer to version that supports data-attr
@mariusandra mariusandra temporarily deployed to posthog-toolbar-element-ie4k12 August 17, 2020 09:07 Inactive
@mariusandra mariusandra temporarily deployed to posthog-toolbar-element-ie4k12 August 17, 2020 10:13 Inactive
@mariusandra mariusandra changed the title [WIP] toolbar element selection improvement [WIP] Toolbar Heatmap & Inspect improvement Aug 17, 2020
@mariusandra mariusandra temporarily deployed to posthog-toolbar-element-ie4k12 August 17, 2020 12:14 Inactive
@mariusandra mariusandra temporarily deployed to posthog-toolbar-element-ie4k12 August 17, 2020 13:00 Inactive
@mariusandra mariusandra temporarily deployed to posthog-toolbar-element-ie4k12 August 17, 2020 13:07 Inactive
@mariusandra mariusandra temporarily deployed to posthog-toolbar-element-ie4k12 August 17, 2020 13:43 Inactive
@mariusandra mariusandra temporarily deployed to posthog-toolbar-element-ie4k12 August 18, 2020 08:13 Inactive
@mariusandra mariusandra marked this pull request as ready for review August 18, 2020 08:26
@mariusandra
Copy link
Collaborator Author

The tests finally pass! (I'm not sure if I did anything TBH...)

Thus this is ready for a review. I updated the original task description with various screenshots of what gets fixed with these changes. Posthog.com looks quite good under the toolbar now!

To elaborate on one change:

"It also upgrades Simmer to a version that uses [data-attr] in its generated selectors when available."

We use data-attr a lot with cypress tests to help hook onto the right elements:

image

... and I believe it's a very useful and powerful abstraction, as it separates style from meaning. We can add subtle hints in the app that help us efficiently automatize software without relying on CSS id and class attributes.

This change basically lets simmer hook into data-attr attributes. Whereas before it would go p:nth-child(30) a:nth-child(2) or so:

image

If you add data-attr, you can now go like this:

image

This can make your designs future proof.

Technically, yes, you could add a very unique id to the attribute, but that has various issues that I won't get into now...

It's especially apparent in the posthog app:

Before:

2020-08-18 10 53 33

After:

2020-08-18 10 50 50

Is this a controversial change... or is this something cool? It is totally optional to add these data-attr of course and we must make sure autocapture works really well under all situations.

@mariusandra mariusandra requested a review from timgl August 18, 2020 09:02
@mariusandra mariusandra changed the title [WIP] Toolbar Heatmap & Inspect improvement Toolbar Heatmap & Inspect improvement Aug 18, 2020
@timgl
Copy link
Collaborator

timgl commented Aug 18, 2020

I think above makes a lot of sense and we should really push people to add data-attr attributes everywhere if they want a robust implementation. Will have a test

@timgl timgl merged commit 1e261eb into master Aug 18, 2020
@timgl timgl deleted the toolbar-element-issues branch August 18, 2020 09:13
@mariusandra
Copy link
Collaborator Author

@yakkomajuri - I think this PR has some implications for the docs.

@yakkomajuri
Copy link
Contributor

Yup, will document how the Toolbar finds elements and how using data-attr is the suggested way for a better toolbar experience

@mariusandra
Copy link
Collaborator Author

Great, thank you! Just to add, please emphasize that data-attr is optional. I really don't want people to walk away with the false impression of "posthog won't work until I add this special attribute everywhere".

Here's some (raw) text that could be of help:

When you create an action to track the usage of a button, PostHog creates a unique CSS selector that pinpoints it, taking into account all unique tags, classes and IDs. Yet the accuracy and reliability of this selector will always depend on your site's markup.

If the best unique selector for a button is div.ant-col.ant-col-xs-8.ant-col-sm-8.ant-col-md-6.ant-col-lg-6.ant-col-x1-6:nth-child(3) > a, you can run into problems. For example, how can you guarantee that :nth-child(3) will still point to the same element? What if .ant-col-sm-8 changes to .ant-col-sm-6 after some design feedback?

To solve this, you can provide additional hints with data-attr.... (+ description)

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.

3 participants