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

Audience UI #36

Merged
merged 102 commits into from
May 4, 2020
Merged

Audience UI #36

merged 102 commits into from
May 4, 2020

Conversation

roborourke
Copy link
Contributor

@roborourke roborourke commented Feb 28, 2020

Still plenty to do and questions to answer. Like should we grab the index mapping to access all the fields so we get their type, or just infer that given we know the data structure...

Fixes #19

TODO:

  • Audience test endpoint
  • Audience save endpoint
  • Handle lack of analytics data

Potentially split these to separate PRs

  • Documentation
  • Handle numbers via ranges eg. gt, lt, gte, lte
  • Handle wildcards / free text
  • Audience post listing columns - estimate spark line / number
  • Audience add/edit control & modal for block editor
  • Assign endpoint to audience client side
  • Provide on updateEndpoint event for external code to hook in and re-render things

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 (1 error).

@roborourke roborourke mentioned this pull request Mar 4, 2020
@roborourke
Copy link
Contributor Author

WIP: Audience builder UI

Audience builder UI in progress.

@roborourke roborourke requested a review from rmccue March 10, 2020 14:23
I had mistakenly used a timestamp, while this was handled by memcached and the memcached dropin the redis cache only expects a TTL in seconds.
@rmccue
Copy link
Member

rmccue commented Mar 12, 2020

Haven't gone through the code here yet, but here's my quick notes from user testing:

  • Remove button needs better styling, a bit off-centre?
    Screen Shot 2020-03-12 at 16 06 05
  • "equals" "Empty" is a bit unclear (could be a browser called Empty eg), should be "is empty" or "equals" "[Empty]" or similar
  • The graph is super slick; we should add a loading icon after the inputs change while waiting for the data to load.
  • Should we rename the "options" box to "audience estimates" to avoid the duplicative title?
  • "This page is asking you to confirm that you want to leave" when clicking save
  • Auto Drafts are listed in the audience table, have "Invalid parameter(s): audience". Related, clicking Trash doesn't remove it from the list; is the post status filter incorrectly selecting all statuses?
  • "The response is not a valid JSON response."; empty backend response? URL is http://altis.local/wp-json/analytics/v1/audiences/estimate?audience=%7B%22include%22%3A%22all%22%2C%22groups%22%3A%5B%7B%22include%22%3A%22any%22%2C%22rules%22%3A%5B%7B%22field%22%3A%22attributes.referer%22%2C%22operator%22%3A%22%21%3D%22%7D%5D%7D%5D%7D&_locale=user
  • On the update page, the value field on the first row isn't visible until you change the comparison. Here's a screen rec of the new page first (which works) and the update page (which doesn't):
    audience-field-bug

This prevents a rendering error if the API response is an error object

Co-Authored-By: Ryan McCue <me@ryanmccue.info>
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.

Overall, the code looks good to me.

In the JS section, there's a lot bundled into one particular component (noted in comment) that could really use with splitting out into multiple instead; both for readability, and for potential reuse in the future.

IMO, the readability of a few of these would also be enhanced by switching them to class components with methods/etc rather than functional components.

README.md Show resolved Hide resolved
composer.json Show resolved Hide resolved
inc/audiences/rest_api/namespace.php Show resolved Hide resolved
inc/audiences/rest_api/namespace.php Outdated Show resolved Hide resolved
inc/audiences/rest_api/namespace.php Outdated Show resolved Hide resolved
src/audiences/edit/index.js Outdated Show resolved Hide resolved
src/audiences/index.js Outdated Show resolved Hide resolved
src/audiences/index.js Outdated Show resolved Hide resolved
src/audiences/index.js Outdated Show resolved Hide resolved
src/audiences/index.js Outdated Show resolved Hide resolved
rmccue and others added 2 commits May 4, 2020 14:37
@roborourke roborourke merged commit 3c927d3 into master May 4, 2020
@roborourke roborourke deleted the audience-ui branch May 4, 2020 15:10
@roborourke
Copy link
Contributor Author

Oh thank god for that. Alright, full steam ahead on XBs!

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.

Audience UI
2 participants