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

Migrate Cohort visualization to React #4270

Merged
merged 12 commits into from
Nov 13, 2019
Merged

Migrate Cohort visualization to React #4270

merged 12 commits into from
Nov 13, 2019

Conversation

kravets-levko
Copy link
Collaborator

@kravets-levko kravets-levko commented Oct 21, 2019

What type of PR is this? (check all applicable)

  • Refactor

Description

  • Convert component to React
    • Renderer
    • Cornelius library
    • Editor
  • Refine and improvements
    • Cornelius: dynamic colors (using chroma-js)
    • Cornelius: format numbers with numeral (no editor options for now)
    • Cornelius: moment for date formatting and manipulations + remove hard-coded month names
    • Cornelius: revisit styles
  • Cleanup
  • Tests

Related Tickets & Documents

#3301 (Migrate Visualizations to React -> Cohort)
Fixes #4329

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

image
image

@kravets-levko kravets-levko added Frontend Visualizations Frontend: React Frontend codebase migration to React labels Oct 21, 2019
@kravets-levko kravets-levko self-assigned this Oct 21, 2019
@arikfr arikfr mentioned this pull request Oct 21, 2019
24 tasks
@kravets-levko kravets-levko changed the title Migrate cohort to react Migrate Cohort visualization to React Oct 21, 2019
@deecay
Copy link
Contributor

deecay commented Oct 22, 2019

Hi @kravets-levko,

Very nice changes. While you are at it, I have two suggestions.

  1. Change Column title from "People" to "Population" to make it even more general?
  2. When screen is wide, the chart would not occupy the whole width by expanding the "Time" column.

@kravets-levko
Copy link
Collaborator Author

kravets-levko commented Oct 22, 2019

Hi @deecay! Thank you for a feedback.

Change Column title from "People" to "Population" to make it even more general?

"People" is an accidental bug (I was working late night yesterday 😴) - restored "Users".

Actually, as you may notice, I prepare a ground for more configuration options for this visualizations (like changing titles, labels, value formats, colors) - they're now supported in rendering engine, but not available in UI yet because I don't want to introduce too much changes and new features in this PR.

When screen is wide, the chart would not occupy the whole width by expanding the "Time" column.

That was also leftover after some experiments - restored previous behavior:
image

@kravets-levko kravets-levko marked this pull request as ready for review October 22, 2019 09:18
@deecay
Copy link
Contributor

deecay commented Oct 22, 2019

@kravets-levko Nice. Thanks.

@arikfr arikfr merged commit c004107 into master Nov 13, 2019
@arikfr arikfr deleted the migrate-cohort-to-react branch November 13, 2019 12:39
@arikfr
Copy link
Member

arikfr commented Nov 13, 2019

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend: React Frontend codebase migration to React Frontend Visualizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cohort visualization isn't using the same font as the rest of the application
4 participants