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

Experience Blocks #24

Merged
merged 44 commits into from
Jun 5, 2020
Merged

Experience Blocks #24

merged 44 commits into from
Jun 5, 2020

Conversation

roborourke
Copy link
Collaborator

@roborourke roborourke commented May 11, 2020

This is the initial version of experience blocks. These are conditional content blocks backed by analytics data audiences.

Copy link

@hm-linter hm-linter bot left a comment

Choose a reason for hiding this comment

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

Linting failed (75 errors, 5 warnings).

(1 notice occurred in your codebase, but were on files/lines not included in this PR.)

@roborourke roborourke marked this pull request as draft May 12, 2020 08:38
@roborourke
Copy link
Collaborator Author

Screenshot 2020-05-12 10 09 06

Screenshot 2020-05-12 10 08 30

Screenshot 2020-05-12 10 10 14

Screenshot 2020-05-12 10 10 46

@roborourke roborourke marked this pull request as ready for review May 16, 2020 16:01
@roborourke
Copy link
Collaborator Author

I've polished up the UI considerably and added a copy function for quickly cloning an existin variant.

  • With lots of variants the toolbar will start scrolling horizontally
  • Variant selection uses a consistent UI with block toolbar buttons
  • Delete & copy buttons now in the shaded XB header zone & hidden when block is not focused
  • Audience picker is now the exported one from the analytics parent plugin

@roborourke roborourke changed the title WIP: Experience Blocks Experience Blocks May 16, 2020
inc/features/blocks/experience/register.php Outdated Show resolved Hide resolved
inc/features/blocks/experience/register.php Outdated Show resolved Hide resolved
inc/features/blocks/experience/register.php Outdated Show resolved Hide resolved
src/experiments.js Outdated Show resolved Hide resolved
src/experiments.js Outdated Show resolved Hide resolved
@joehoyle
Copy link
Member

With no Audiences created, I get: . Also I still get that after creating 1 active audience actually too.

I think due to

@joehoyle
Copy link
Member

Ok, inserting working! Couple Q's / observations:

  1. How / can I select a "default" audience variations? I tried just not selecting any audience, but on the front end I don't see anything:

  2. Hover on the active tab changes to border-only, which is odd (I'm hovering the active tab here)

  3. The ALL CAPS on the XB title seems quite out of place, I don't think the editor UI uses all caps so seems odd.

  4. Would it make sense to add the selected audience name after "VARIANT 3", in my case "Variant 3 - Firefox" to tie them together a bit more.

@roborourke
Copy link
Collaborator Author

  1. How / can I select a "default" audience variations? I tried just not selecting any audience, but on the front end I don't see anything

That's my bad I just forgot to factor that in yet, I think I'm going to add some text on the left to say 'If no audience is selected this variant will be used as a fallback' and make sure the front end deals with that.

  1. Hover on the active tab changes to border-only, which is odd (I'm hovering the active tab here)

Ah yeah, the whole IconButton CSS is weird, will fix.

  1. The ALL CAPS on the XB title seems quite out of place, I don't think the editor UI uses all caps so seems odd.

Fair enough! Will change that, just wanted it to stand out.

  1. Would it make sense to add the selected audience name after "VARIANT 3", in my case "Variant 3 - Firefox" to tie them together a bit more.

I actually wanted to use the audience name in place of variant X altogether. Might need to rethink that if we go for selecting multiple audiences at some point though. I'll put this in for now as I agree it makes sense.

@roborourke
Copy link
Collaborator Author

@joehoyle ok, addressed all of those comments now

@roborourke
Copy link
Collaborator Author

don't know how possible this is to fix but: if you use a theme which supports wide/full width images (such as Twenty Twenty), images inside XBs won't expand to the correct width.
Twenty Twenty at least is using a direct descendent selector, so may not be possible to fix.

The experience block itself is a container so I added alignment support directly to it - you'd have to align that full / wide first.

@rmccue
Copy link
Member

rmccue commented May 28, 2020

The experience block itself is a container so I added alignment support directly to it - you'd have to align that full / wide first.

Yeah, I did, the problem is the selectors in the theme are using direct descendent selectors, so... probably not possible.

@roborourke
Copy link
Collaborator Author

roborourke commented May 29, 2020

There's really just the issue of deleting audiences left to figure out now but I can't work out the right approach just yet.

There are a few options I've thought about:

  1. Don't allow deleting audiences :/
    • Could potentially leave a revision / orphaned audience behind
    • Deactivating could be the only way to "remove" them
  2. When an audience is selected add a post meta value with post type & ID (or taxonomy term?)
    • This is going to be difficult to remove when a different audience gets selected
    • Would allow prevention of deleting audiences that are in use
  3. Store the REST API not found error response in the redux store and check for this value
    • Won't allow prevention of deleting
    • Only handles missing audience situation in the XB UI

Any better ideas?

@rmccue
Copy link
Member

rmccue commented Jun 1, 2020

I'd say option 3 is best, something in the UI like "Audience has been deleted".

Comment on lines +14 to +15
require_once __DIR__ . '/personalization/register.php';
require_once __DIR__ . '/personalization-variant/register.php';
Copy link
Member

Choose a reason for hiding this comment

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

I think these should ideally be in the main plugin.php; if this is a pattern already in the other "features", that's OK, but we should switch in a future PR.

inc/features/blocks/namespace.php Show resolved Hide resolved
inc/features/blocks/personalization-variant/edit.js Outdated Show resolved Hide resolved
inc/features/blocks/personalization-variant/edit.js Outdated Show resolved Hide resolved
inc/features/blocks/personalization-variant/edit.js Outdated Show resolved Hide resolved
inc/features/blocks/personalization/register.php Outdated Show resolved Hide resolved
inc/features/blocks/personalization/register.php Outdated Show resolved Hide resolved
inc/features/blocks/personalization/register.php Outdated Show resolved Hide resolved
src/experiments.js Outdated Show resolved Hide resolved
webpack.config.js Show resolved Hide resolved
Copy link
Member

@rmccue rmccue left a comment

Choose a reason for hiding this comment

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

Small changes; almost all have suggestions :)

.eslintrc Show resolved Hide resolved
inc/features/blocks/personalization-variant/edit.js Outdated Show resolved Hide resolved
inc/features/blocks/personalization-variant/edit.js Outdated Show resolved Hide resolved
inc/utils/namespace.php Outdated Show resolved Hide resolved
plugin.php Outdated Show resolved Hide resolved
inc/features/titles/namespace.php Outdated Show resolved Hide resolved
Copy link
Member

@rmccue rmccue left a comment

Choose a reason for hiding this comment

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

J'approve. :wotf-ship:

@roborourke roborourke merged commit c9e12b1 into master Jun 5, 2020
@roborourke roborourke deleted the experience-blocks branch June 5, 2020 16:07
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