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

[HOLD] feat(popups): default segments UI #1524

Closed
wants to merge 9 commits into from
Closed

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented Feb 23, 2022

All Submissions:

Changes proposed in this Pull Request:

UI for Automattic/newspack-popups#809.

How to test the changes in this Pull Request:

See testing instructions in Automattic/newspack-popups#809.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@dkoo dkoo self-assigned this Feb 23, 2022
@dkoo dkoo added [Type] Feature request Request for new features [Status] Needs Review The issue or pull request needs to be reviewed labels Feb 23, 2022
@dkoo dkoo changed the title feat(popups): add button to generate or reset default segments feat(popups): default segments UI Feb 23, 2022
@dkoo dkoo marked this pull request as ready for review February 23, 2022 00:24
@dkoo dkoo requested a review from a team as a code owner February 23, 2022 00:24
Copy link
Member

@adekbadek adekbadek left a comment

Choose a reason for hiding this comment

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

This would use @thomasguillot 's input, but I think there's a problem with CTA hierarchy here:

image

The "Generate…" message should be more prominent IMO – perhaps centered and framed?

image

assets/wizards/popups/views/segments/SegmentsList.js Outdated Show resolved Hide resolved
assets/wizards/popups/views/segments/SegmentsList.js Outdated Show resolved Hide resolved
@thomasguillot
Copy link
Contributor

I agree with @adekbadek, there are too many Primary buttons.

Segments - Empty

@dkoo
Copy link
Contributor Author

dkoo commented Feb 23, 2022

I like the proposed design a lot better than my single button! One thing to note, there's discussion in Automattic/newspack-popups#809 about automatically triggering the default segment creation if the site lacks any active prompts and segments, so that would mean the proposed UI would only really be shown for sites that are running active campaigns but haven't created any segments.

For sites that may have active campaigns and segments already, how should the button be presented? As a secondary button next to the "Add New Segment" button? I thought about putting a popover menu button next to it, but those components don't quite work in this context, outside of an action card component.

@dkoo
Copy link
Contributor Author

dkoo commented Feb 23, 2022

@adekbadek @thomasguillot I pushed some changes in 18cbc23 to update the design as proposed. If we decide to auto-generate these default segments for new sites, I'll make that change here.

@dkoo dkoo requested a review from adekbadek February 23, 2022 23:18
@thomasguillot
Copy link
Contributor

@dkoo I pushed a few more visual changes (20254be)

wizard-updated

@thomasguillot
Copy link
Contributor

thomasguillot commented Feb 24, 2022 via email

@dkoo
Copy link
Contributor Author

dkoo commented Feb 24, 2022

It's been decided to automatically generate the default segments on WP init rather than via the UI as long as the site has no pre-existing segments. This means that @thomasguillot's lovely new UI will only ever be seen as a fallback in case this creation fails for whatever reason.

174f966 adds an additional check and error message if the required Campaigns config file is missing—I noticed that on non-Atomic sites, there's no visual warning in the wizard UI that we're missing the file and can't use Campaigns features.

Copy link
Member

@adekbadek adekbadek left a comment

Choose a reason for hiding this comment

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

Note: changes here depend on Automattic/newspack-popups#809, these should be merged simultaneously.

@github-actions github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Feb 25, 2022
@dkoo
Copy link
Contributor Author

dkoo commented Feb 25, 2022

Note to @thomasguillot: after some discussion we've decided we'd like to build this out into a more guided onboarding/wizard experience to guide users toward one of several sets of default segments, based on their input. @aschweigert will work on a suggested flow for this, but let's hold these changes until we can flesh that out a bit more.

@dkoo dkoo changed the title feat(popups): default segments UI [HOLD] feat(popups): default segments UI Feb 25, 2022
@dkoo
Copy link
Contributor Author

dkoo commented Apr 11, 2022

Closing this as we've decided to go in a different direction for establishing defaults.

@dkoo dkoo closed this Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Approved The pull request has been reviewed and is ready to merge [Status] Needs Design Review [Status] On Hold [Type] Feature request Request for new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants