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 Word Cloud visualization to React #3930

Merged
merged 15 commits into from
Jul 3, 2019

Conversation

kravets-levko
Copy link
Collaborator

@kravets-levko kravets-levko commented Jun 25, 2019

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

  • Refactor
  • Feature
  • Bug Fix

Description

Related Tickets & Documents

#3301 (Migrate Visualizations to React -> Word Cloud)

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

Query page:
image

New editor options:
image
image

Dashboard:
image

@kravets-levko kravets-levko marked this pull request as ready for review June 28, 2019 18:20
@kravets-levko
Copy link
Collaborator Author

@arikfr @ranbena @gabrieldutra This PR is ready for review - I'll appreciate any feedback. I'm still testing it, and will remove WIP label once I'm finally happy with everything.

@kravets-levko
Copy link
Collaborator Author

@mohamagdy Since this PR is going to replace yours (#3668) I'll appreciate your feedback as well (sorry, cannot request your review from GitHub UI)

Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

👌

client/app/visualizations/chart/index.js Outdated Show resolved Hide resolved
client/app/visualizations/word-cloud/Editor.jsx Outdated Show resolved Hide resolved
client/app/visualizations/word-cloud/Editor.jsx Outdated Show resolved Hide resolved
client/app/visualizations/word-cloud/Editor.jsx Outdated Show resolved Hide resolved
client/app/visualizations/word-cloud/Editor.jsx Outdated Show resolved Hide resolved
client/app/visualizations/word-cloud/Editor.jsx Outdated Show resolved Hide resolved
client/app/visualizations/word-cloud/Editor.jsx Outdated Show resolved Hide resolved
client/app/visualizations/word-cloud/Editor.jsx Outdated Show resolved Hide resolved
client/app/visualizations/word-cloud/Editor.jsx Outdated Show resolved Hide resolved
@kravets-levko kravets-levko force-pushed the migrate-visualizations-word-cloud branch from 720c7b5 to ead140f Compare June 30, 2019 10:15
Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

👌

@kravets-levko kravets-levko changed the title (WIP) Migrate Word Cloud visualization to React Migrate Word Cloud visualization to React Jun 30, 2019
client/app/lib/hooks/useQueryResult.js Outdated Show resolved Hide resolved
client/app/services/resizeObserver.js Outdated Show resolved Hide resolved
@ranbena
Copy link
Contributor

ranbena commented Jul 1, 2019

I think the only thing missing now is a simple Percy test for it.

@kravets-levko
Copy link
Collaborator Author

@ranbena Added some tests

@kravets-levko kravets-levko force-pushed the migrate-visualizations-word-cloud branch 3 times, most recently from 660c589 to 6b32a47 Compare July 2, 2019 12:02
@kravets-levko kravets-levko force-pushed the migrate-visualizations-word-cloud branch from 6b32a47 to 7c78ada Compare July 2, 2019 12:26
Copy link
Contributor

@ranbena ranbena left a comment

Choose a reason for hiding this comment

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

Perfection

@kravets-levko
Copy link
Collaborator Author

@ranbena Thanks! There are some issues with Percy (visualization is messed up on screenshots), I'm trying to figure this out. After that I'll merge this PR.

@kravets-levko kravets-levko force-pushed the migrate-visualizations-word-cloud branch 2 times, most recently from 5d3bc30 to 8192527 Compare July 2, 2019 16:37
@kravets-levko kravets-levko force-pushed the migrate-visualizations-word-cloud branch from 8192527 to 17aacc1 Compare July 2, 2019 18:56
@kravets-levko
Copy link
Collaborator Author

kravets-levko commented Jul 2, 2019

@arikfr @ranbena @gabrieldutra

There is an issue with this visualization and Percy, which was quite hard to figure out (even required a help from Percy support). Word Cloud used an "Impact" font to compute layout, and later the same font was assigned to all words. But "Impact" is not available on all machines, so browser uses some fallback. To take a screenshot of a page, Percy takes a complete DOM snapshot with all loaded resources and inline CSS, copies it to own machines and then loads in own browsers. On CI machines word cloud was computed using some default font, and after copying DOM to Percy - restored with another default font available on that machine:

image
Words are messed up because Percy's browser uses different font with a different size of letter

The only solution which 100% worked was to use some fixed font, which will be exactly the same on all machines. I tried Roboto because we already have it (however, don't use since #2081), but before I merge it - I decided to start a discussion to be sure that I'm not missing anything important.

image
Using Roboto font

Upd.: This behavior is totally not a problem for users, because when user changes OS/browser - they will reload page and visualization will re-build using another font. The issue is only when using CI and Percy together (because of copying DOM instead of reloading page in Percy or taking screenshot on CI machines)

@arikfr
Copy link
Member

arikfr commented Jul 2, 2019

@kravets-levko good catch! How about we define a Percy only CSS rule that uses Roboto, but keep things for users unchanged?

@kravets-levko
Copy link
Collaborator Author

@arikfr We need to detect Cypress env as well - because Roboto should be loaded there for proper layout calculation. Not sure if there is any "legal" way to target Cypress and Percy 🤔

@kravets-levko
Copy link
Collaborator Author

I think I can add some env to Cypress dockerfile and then use it to conditionally add fonts during webpack build:

PYTHONUNBUFFERED: 0
REDASH_LOG_LEVEL: "INFO"
REDASH_REDIS_URL: "redis://redis:6379/0"
REDASH_DATABASE_URL: "postgresql://postgres@postgres/postgres"
REDASH_RATELIMIT_ENABLED: "false"

@gabrieldutra Do you have any better ideas?

@arikfr
Copy link
Member

arikfr commented Jul 3, 2019

We need to detect Cypress env as well - because Roboto should be loaded there for proper layout calculation. Not sure if there is any "legal" way to target Cypress and Percy 🤔

In the test code we have full control of the browser, why not change the CSS from code? Feels safer and easier than passing around from the Dockerfile.

@kravets-levko
Copy link
Collaborator Author

That's interesting idea. If Percy will pick that injected font - then it should work. Need to try

@kravets-levko
Copy link
Collaborator Author

@arikfr Yay, it works! I needed to update webpack config to copy Roboto files to dist/ without referencing them anywhere, but they won't load anywhere except of CI. Can you please take a last look before I merge it? Thanks!

@arikfr
Copy link
Member

arikfr commented Jul 3, 2019

Looks good 👍

@kravets-levko kravets-levko merged commit cc48de0 into master Jul 3, 2019
@kravets-levko kravets-levko deleted the migrate-visualizations-word-cloud branch July 3, 2019 10:29
@arikfr arikfr mentioned this pull request Jul 3, 2019
24 tasks
@kravets-levko kravets-levko added the Frontend: React Frontend codebase migration to React label Jul 8, 2019
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Nov 14, 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.

WordCloud hides words that don't fit in the container
4 participants