Skip to content

Conversation

alexkim205
Copy link
Contributor

Problem

https://posthog.slack.com/archives/C034XD440RK/p1652294642784479

Changes

Before

Screen Shot 2022-05-11 at 3 09 36 PM

After

Screen Shot 2022-05-11 at 3 08 10 PM

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

con mi ojos

@alexkim205 alexkim205 self-assigned this May 11, 2022
@alexkim205 alexkim205 requested review from clarkus and EDsCODE May 11, 2022 19:14
Copy link
Contributor

@clarkus clarkus left a comment

Choose a reason for hiding this comment

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

Looks good - just noticed that we could update the error content to be a bit more clear and less about the user. Instead of saying:

Error: you need to set a name

We could say something like:

Cohort name cannot be empty

It just makes it more about the required field and less about what the user did.

@alexkim205 alexkim205 requested a review from clarkus May 11, 2022 19:29
@alexkim205 alexkim205 enabled auto-merge (squash) May 11, 2022 19:31
@alexkim205 alexkim205 merged commit c0ff307 into master May 11, 2022
@alexkim205 alexkim205 deleted the fix/error-cohort-name-field branch May 11, 2022 19:38
fuziontech added a commit that referenced this pull request May 12, 2022
* master:
  fix(cohorts): num comparison (#9764)
  fix(trends): Use 0 as fallback data point value (#9766)
  fix: Flexing of PropertyFilterButton (#9761)
  chore(funnels): Add `lemon-funnel-viz` to `PERSISTED_FEATURE_FLAGS` (#9757)
  feat(insights): New and improved ActionFilter (#9741)
  fix: make the kafka inspector work again after kea-forms update (#9758)
  fix: repace underscores with spaces in the cloud announcement (#9751)
  chore(contributors): 🤖 - Add utkuzih as a contributor 🎉 (#9748)
  fix: cohort person property value not populated (#9745)
  fix: make cohort name field error consistent (#9744)
  fix(cohort): don't let time_value be less than 0 (#9742)
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.

2 participants