[ZEPPELIN-6265] Support classic Helium viz packages in new UI#5006
Conversation
| @@ -0,0 +1,28 @@ | |||
| import { HeliumPackageType } from '@zeppelin/helium'; | |||
There was a problem hiding this comment.
Could you please add the Apache License header at the top of this file? This ensures compliance with ASF requirements. Thanks!
There was a problem hiding this comment.
Thank you for checking! I've fixed it with other Rat-check violations.
| icon: string; | ||
| type: HeliumType; | ||
| // tslint:disable-next-line:no-any | ||
| class: any; |
There was a problem hiding this comment.
How about use unknown (with type narrowing)?
| constructor() {} | ||
|
|
||
| // tslint:disable-next-line:no-any | ||
| addDragDropDirectives(module: any): void { |
There was a problem hiding this comment.
How about use IModule type from @types/angular?
| function(dragDropService: any) { | ||
| return { | ||
| restrict: 'A', | ||
| // tslint:disable-next-line:no-any | ||
| link: function(scope: any, element: any, attrs: any) { |
There was a problem hiding this comment.
How about removing the explicit any? Type inference would work correctly here.
|
@seung-00 Added new interfaces instead of reusing the Visualization class from zeppelin-vis, since Helium packages are eval’d at runtime and their prototype chains don’t match. Interfaces were preferred over classes because only the shape is shared across eval’d definitions. |
| module.service('customDragDropService', [ | ||
| '$parse', | ||
| // tslint:disable-next-line:no-any | ||
| function($parse: any) { |
There was a problem hiding this comment.
Would it be hard to use the types provided by @types/angular here as well?
And I'm not fully sure, but if the TS compiler can infer the type in some places without explicitly declaring it, it might be nice to rely on inference.
There was a problem hiding this comment.
Thanks for checking! I'll update it to utilize TS type inference.
| this.columns = updatedClassicData.columns; | ||
| this.rows = updatedClassicData.rows; | ||
| this.comment = updatedClassicData.comment; | ||
| }.bind(this) |
There was a problem hiding this comment.
How about using an arrow function instead of bind? I think it's more readable and more modern way.
| const updatedClassicData = this.convertToClassicFormat(modernTableData); | ||
| this.columns = updatedClassicData.columns; | ||
| this.rows = updatedClassicData.rows; | ||
| this.comment = updatedClassicData.comment; |
There was a problem hiding this comment.
I'm not sure, but it looks like the proxy was supposed to be updated. However, it seems that the TableDataAdapterService instance, which is bound, is being updated instead.
There was a problem hiding this comment.
You're right, the this binding was incorrect here. So I changed it to use arrow functions and access through variable names instead.
8410ad5 to
840c9c3
Compare
|
@seung-00 I've checked for places where type inference can be used as suggested in the review, and fixed the incorrect |
Reamer
left a comment
There was a problem hiding this comment.
A minor comment on the backend code. I am unable to review the frontend part.
|
Rebased onto the master branch and add some Prttier exclusion rules for |
b1bdabf to
5b4529a
Compare
|
@tbonelee Please rebase onto the latest master to resolve the conflicts. |
# Conflicts: # zeppelin-web-angular/src/app/helium-manager/public-api.ts # zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.ts # zeppelin-web-angular/src/app/pages/workspace/workspace.component.ts
…ormation settings
…ion settings and import font stylesheet
…instead of `any`
…terface and remove unnecessary `any` types
…service` with `factory`
# Conflicts: # zeppelin-web-angular/.prettierignore
5b4529a to
99673d7
Compare
|
Rebased it onto the master branch |
### What is this PR for? This PR enables the use of external Helium visualization packages, which are previously available only in the classic UI, in the new UI. The process of loading and importing Helium packages, as well as rendering them, closely follows the logic from the classic UI. Since the new-style visualization classes in the new UI are not compatible with the existing ones, the implementation branches accordingly to handle both. Classic Helium visualization packages depend on legacy technologies such as AngularJS, jQuery, and several visualization libraries, which had to be included as a result. For Bootstrap styles, conflicts arose with existing styles, so I copied and modified the original HTML templates to prevent clashes. If a Helium package attempts to use the original HTML template, it will be replaced with the modified version. Additionally, I identified that the Helium visualization support classes from the discontinued development of the new UI are no longer in use, so I removed them. ### What type of PR is it? Improvement ### Todos * [v] - Added support for loading Helium packages * [v] - Fixed style mismatches in Helium packages * [v] - Other functional issues resolved ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-6265 ### How should this be tested? * Enables a classic Helium visualization package in the classic UI, and verify that it works in the new UI. - Note: The `/helium` package management page has not yet been added. ### Questions: * Does the license files need to update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Closes #5006 from tbonelee/helium-vis. Signed-off-by: ChanHo Lee <chanholee@apache.org> (cherry picked from commit 948f165) Signed-off-by: ChanHo Lee <chanholee@apache.org>
|
Thanks for the reviews, I've merged this into master and branch-0.12 |
What is this PR for?
This PR enables the use of external Helium visualization packages, which are previously available only in the classic UI, in the new UI.
The process of loading and importing Helium packages, as well as rendering them, closely follows the logic from the classic UI.
Since the new-style visualization classes in the new UI are not compatible with the existing ones, the implementation branches accordingly to handle both.
Classic Helium visualization packages depend on legacy technologies such as AngularJS, jQuery, and several visualization libraries, which had to be included as a result.
For Bootstrap styles, conflicts arose with existing styles, so I copied and modified the original HTML templates to prevent clashes.
If a Helium package attempts to use the original HTML template, it will be replaced with the modified version.
Additionally, I identified that the Helium visualization support classes from the discontinued development of the new UI are no longer in use, so I removed them.
What type of PR is it?
Improvement
Todos
What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-6265
How should this be tested?
/heliumpackage management page has not yet been added.Questions: