-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
feat: implement new version of word cloud #9962
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9962 +/- ##
==========================================
+ Coverage 71.35% 71.37% +0.01%
==========================================
Files 585 585
Lines 30913 30914 +1
Branches 3246 3246
==========================================
+ Hits 22059 22064 +5
+ Misses 8744 8741 -3
+ Partials 110 109 -1
Continue to review full report at Codecov.
|
61689a2
to
fcef98c
Compare
fcef98c
to
71123fe
Compare
@@ -23,7 +23,7 @@ import DashboardFilterTest from './filter'; | |||
import DashboardLoadTest from './load'; | |||
import DashboardSaveTest from './save'; | |||
import DashboardTabsTest from './tabs'; | |||
import DashboardUrlParamsTest from './url_params'; | |||
import DashboardFormDataTest from './url_params'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to rename this file to formData.js
, but a recently added linting rule preferring TypeScript threw a weird exception that I didn't have time to debug now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you get a screenshot of the exception? It's supposed to post a comment but not block merge if a new js file is added, maybe i missed something though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etr2460 I don't think it was throwing a normal comment, it looked more like it borked somehow. Let me redo the rename so we can get a screenshot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yikes... so one of the actions the workflow was depending on didn't work :( it doesn't block the PR merge though, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like someone else reported this as well (2 hours ago) trilom/file-changes-action#100. I'll wait a few days to see if it gets resolved, otherwise, i can remove the workflow and try to fix it since it's not required to pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good to know it's a known bug. I can do the rename in a separate PR if we want to debug this further, but for now I'll leave the rename out of this PR.
d92f25a
to
71123fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Hi @villebro @rusackas , I was migrating table chart to use the new API. The more I look at it, the more I feel there shouldn't be the need to include this The information about field mapping is at the visualization type level, and isn't a variable per visualization. There is already a |
@ktmud My original idea behind adding the |
SUMMARY
This is a new attempt at bumping Word Cloud to the newest version which was attempted in #9908 . This adds both cypress and jest tests to make sure chart calls initiated from the dashboard contain the property
queryFields
and that the field is dynamically created based on the current control panel state which might change over time.TEST PLAN
Local testing + CI
ADDITIONAL INFORMATION