AF-2700: Create Heatmap internal component#1080
Conversation
jesuino
force-pushed
the
AF-2700
branch
6 times, most recently
from
79921f4 to
92cc3d7
Compare
|
Hello @caponetto and @tiagobento I added you as reviewer because most part of this PR is JS with almost same stack used in Kogito Tooling. Kindly let me know your thoughts about this PR. Hello @sthundat I added you as a reviewer of this PR - if you think this is a mistake please let me know. I will have bits for testing by tomorrow if the CI server fails. Thanks! |
|
jenkins execute fdb |
|
jenkins execute fdb |
| import org.uberfire.ext.plugin.client.perspective.editor.events.PerspectiveEditorFocusEvent; | ||
|
|
||
| @ApplicationScoped | ||
| public class ComponentsGroupProducer { |
There was a problem hiding this comment.
| public class ComponentsGroupProducer { | |
| public class ComponentGroupProducer { |
| when(externalComponentLoader.loadExternal()).thenReturn(asList(component("c1"))); | ||
|
|
||
| when(layoutComponentsHelper.findComponentsInTemplates((any()))).thenReturn(asList("c1")); | ||
There was a problem hiding this comment.
Please remove these extra spaces.
| import org.uberfire.ext.layout.editor.api.editor.LayoutTemplate; | ||
|
|
||
| @ApplicationScoped | ||
| public class LayoutComponentsHelper { |
There was a problem hiding this comment.
| public class LayoutComponentsHelper { | |
| public class LayoutComponentHelper { |
| ], | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "https://github.com/kiegroup/kogito-tooling.git" |
There was a problem hiding this comment.
Please update this URL.
Also, I don't see this information in the other package.json files.
| componentService.call((List<ExternalComponent> components) -> addExternalComponents(components)) | ||
| .listExternalComponents(); | ||
|
|
||
| componentService.call((List<ExternalComponent> components) -> addInternalComponents(components)) | ||
| .listProvidedComponents(); |
There was a problem hiding this comment.
Can we have a boolean property like isProvided for ExternalComponent?
Then componentService can return all components in a single remote call.
Then you can filter them by isProvided to decide where to add them.
See, the point here is to do only one remote call instead of two.
There was a problem hiding this comment.
The issue I faced with the flag is that users could set the flag in their component's JSON - messing with our internal categories.
But if I only use this flag for internal communication I can force the flag when listing external to false and to true when listing provided components, so I will make this change!
| import { Heatmap } from "heatmap.js"; | ||
| import * as React from "react"; | ||
| import { useEffect, createRef, useState } from "react"; | ||
| import * as heatmap from "heatmap.js"; |
There was a problem hiding this comment.
| import * as heatmap from "heatmap.js"; | |
| import * as heatmap from "heatmap"; |
There was a problem hiding this comment.
I think "heatmap.js" is the name of the module, so in this case, the .js part is required.
| const NOT_ENOUGH_COLUMNS_MSG = "Heatmap expects 2 columns: Node ID (TEXT or Label) and value (NUMBER)"; | ||
| const INVALID_COLUMNS_TYPE_MSG = "Wrong columns type. First column should be TEXT or LABEL and second column NUMBER."; | ||
| const MISSING_PARAM_MSG = "You must provide either a SVG URL or the SVG Content."; |
There was a problem hiding this comment.
Not that I'm saying to do this in this PR, but I think this could be a good opportunity to use our i18n package. :)
Also applicable to other places.
There was a problem hiding this comment.
Indeed we had a brief discussion about it. The near future is to glue components API with Kogito tooling and reuse the protocol and also all available packages available there!
| if (ds.columns.length < 2) { | ||
| return NOT_ENOUGH_COLUMNS_MSG; | ||
| } | ||
| if ((ds.columns[0].type !== "TEXT" && ds.columns[0].type !== "LABEL") || ds.columns[1].type !== "NUMBER") { |
There was a problem hiding this comment.
Can we use ColumnType here instead of "TEXT", "LABEL", and "NUMBER"?
| export function SVGHeatmap(props: SVGHeatmapProps) { | ||
| const parentRef = createRef<HTMLDivElement>(); | ||
| const [svgHeatmap, setSvgHeatmap] = useState<Heatmap<any, any, any>>(); | ||
| const [ repaint, setRepaint ] = useState(false); |
There was a problem hiding this comment.
| const [ repaint, setRepaint ] = useState(false); | |
| const [repaint, setRepaint] = useState(false); |
| const processSelectRef = createRef<HTMLSelectElement>(); | ||
| const [selectedValue, setSelectedValue] = useState<SelectedValue>(); | ||
|
|
||
| const onClickTitled = useCallback((e: any) => { |
There was a problem hiding this comment.
| const onClickTitled = useCallback((e: any) => { | |
| const onTitleClicked = useCallback((e: any) => { |
tiagobento
left a comment
tiagobento
left a comment
There was a problem hiding this comment.
Left some comments inline. I don't have much context about the task in general, so I might've missed some nuances. I'd love to know it more in depth, but I guess the time is coming for us to work together on some things, right? :)
Congrats on the work!
| import * as ComponentAPI from "../controller/api"; | ||
| import * as Bus from "../controller/ComponentBus"; | ||
| import { Bus } from "../controller"; | ||
| import * as ComponentAPI from "../controller"; |
There was a problem hiding this comment.
A file called "controller" exporting something labeled as "ComponentAPI" doesn't smell good. I don't know the context of that, but do you think we can improve it?
There was a problem hiding this comment.
Hello Tiago,
I do agree with you. This is what the API should provide to component developers :
- Model Objects that represents what is sent in the wire:
- DataSet
- FunctionRequest
- FilterRequest
and others objects
- Access to ComponentController, which is how users will communicate with DB. In practice only "getComponentController" would be required so users can set their callback methods and call functions/fire configuration issues alert and more. To expose this function I simply exported it from controller, but I moved api.ts to the root dir, it will avoid confusion.
When importing it I usually call it ComponentAPI and in the component itself users should not see controller package, I hope only the getComponentController and the model objects would be accessible!
| public send(message: ComponentMessage): void { | ||
| console.debug("[BrowserComponentBus] Sending Message"); | ||
| console.debug(message); | ||
| message.properties.set(MessageProperty.COMPONENT_ID, this.componentId); |
There was a problem hiding this comment.
Can we have "componentId" be a constructor parameter? That would remove this issue.
| this.bus.setListener(this.messageDispatcher); | ||
| this.bus.start(); |
There was a problem hiding this comment.
I can't use in the constructor because the componentId is given by DB when the component is initialized. What I did is move the component ID to the controller, who sends message, and it will acquire the componentId after the init and send all messages with it.
If the component id is undefined then something is really wrong with DB - and all the messages will be sent to oblivion and ignored.
| } | ||
| }, [svgHeatmap, props.svgNodesValues, repaint]); | ||
|
|
||
| window.onresize = (e: any) => setRepaint(previous => !previous); |
There was a problem hiding this comment.
This should be an effect. You want to remove this event listener when this component is unmounted, right? So you have to wrap that into an useEffect and have it be registered on the returning function.
| * limitations under the License. | ||
| */ | ||
|
|
||
| export * from "./SVGHeatmap"; No newline at end of file |
There was a problem hiding this comment.
This is nit-picking, but on kogito-tooling we avoid having acronyms all-caps. We would name this file SvgHeatmap.
| ReactDOM.render( | ||
| <SVGHeatmapComponent controller={ComponentAPI.getComponentController()} />, | ||
| document.getElementById("app")! | ||
| ); | ||
|
|
||
| if (ComponentDev) { | ||
| ComponentDev.start(); | ||
| } |
There was a problem hiding this comment.
As you know, I'm not the biggest fan of static stuff. This has the potential of making our lives much harder in the future. If you could remove the static calls to "ComponentAPI", this would be awesome. Since the other one is related to the development environment, I don't think it's that urgent, but it would be great if you could refactor the two \o/
There was a problem hiding this comment.
Hello Tiago!
Thanks for the suggestion. Would you please provide an example of how it could look like? We can refactor for this PR if the change is small.
Thanks!
There was a problem hiding this comment.
Well, instead of ComponentAPI.getComponentController() you could have something like const componentApi = new ComponentApi() and then componentApi.getController()
| import { useState } from "react"; | ||
| import { Logo, LogoProps } from "./Logo"; | ||
| import * as ComponentAPI from "@dashbuilder-js/component-api"; | ||
| import { ComponentController } from "@dashbuilder-js/component-api/dist/controller/ComponentController"; |
There was a problem hiding this comment.
Shouldn't this be import { ComponentController } from "@dashbuilder-js/component-api"; ?
| props.controller.setOnInit(componentProps => { | ||
| setLogoProps({ | ||
| src: (componentProps.get(SRC_PROP) as string) || DEFAULT_SRC, | ||
| width: componentProps.get(WIDTH_PROP) as string, | ||
| height: componentProps.get(HEIGHT_PROP) as string | ||
| }); | ||
| }; | ||
| }); |
There was a problem hiding this comment.
This is a side effect, should be wrapped in a useEffect call.
| const common = require("../../webpack.common.config"); | ||
|
|
||
| module.exports = async (env, argv) => { | ||
| let entryPoint = "./src/index.tsx"; |
There was a problem hiding this comment.
I think moving that to the "else" part makes for better code, since you're essentially removing mutabilityl :)
There was a problem hiding this comment.
Could you please elaborate what should I added in the else part? In prod build it should use the index without the component dev API, in the dev build it uses index-dev.tsx
| props.controller.setOnInit((params: Map<string, string>) => { | ||
| const validationMessage = validateParams(params); | ||
| if (validationMessage) { | ||
| setAppState(previousAppState => ({ | ||
| ...previousAppState, | ||
| state: AppStateType.ERROR, | ||
| message: validationMessage, | ||
| configurationIssue: validationMessage | ||
| })); | ||
| } else { | ||
| setAppState(previousAppState => ({ | ||
| ...previousAppState, | ||
| state: AppStateType.LOADING_SVG, | ||
| svgRequest: { | ||
| functionName: "ProcessSVGFunction", | ||
| parameters: params | ||
| }, | ||
| configurationIssue: "" | ||
| })); | ||
| } | ||
| }); |
@jesuino ,No problem .I will review this PR |
|
Hello @tiagobento and @caponetto Thanks for your review. I made all the requested changes, some I had some questions which I asked in your comment. Please let me know if you see anything else that needs to be changed. Hello @sthundat I found a bug with the export which I fixed in my last commit. The bug is: the export failed when exporting data with provided components. It should be fixed now. Thanks! |
|
jenkins execute cdb |
|
jenkins execute fdb |
|
jenkins execute fdb |
|
jenkins execute cdb |
|
jenkins execute fdb |
|
Hello @jesuino , |
|
Hello @sthundat Thanks for the feedback. Kindly bear in mind that these issues aren't directly related to heatmap itself, but related to DB or the external components API. I will explain. About 1 and 2 I could not reproduce. Kindly let me know the steps to reproduce and I will open a JIRA for it.
The unexpected error you see in the GIF above is a known use for the Remote Server dataset: it fails to group columns when we repeat a column in the columns list. That can be fixed in a future release as well -> https://issues.redhat.com/browse/AF-2751 Regarding 3, I agree with you. External Components today does not control what datasets users will use, which is cumbersome. We can workaround this by allowing components to tell what is wrong wit the entered dataset, that's what you see while you don't select the right columns: Another possible way to decrease the impact of this is by providing pre-built dashboard and clear instructions how to install them. In any case we plan to improve this in the near future. You may have noticed that for standard reporting the columns are pre selected. This is done by a mechanism called "Displayer Constraints" and right now this can't be achieved by the external component displayer because the constraints are fixed. In a future release we should be able to declare the constraints in the displayer descriptor (manifest.json) and apply them on the UI, but this need to be carefully discussed and would be considered as a new feature. |
| if (process.env.WEBPACK_DEV_SERVER) { | ||
| entryPoint = "./src/index-dev.tsx"; | ||
| copyResources.push({ from: "./static/manifest.dev.json", to: "./manifest.dev.json" }); | ||
| } |
There was a problem hiding this comment.
| if (process.env.WEBPACK_DEV_SERVER) { | |
| entryPoint = "./src/index-dev.tsx"; | |
| copyResources.push({ from: "./static/manifest.dev.json", to: "./manifest.dev.json" }); | |
| } | |
| if (process.env.WEBPACK_DEV_SERVER) { | |
| entryPoint = "./src/index-dev.tsx"; | |
| copyResources.push({ from: "./static/manifest.dev.json", to: "./manifest.dev.json" }); | |
| } else { | |
| entryPoint = "./src/index.tsx"; | |
| } |
There was a problem hiding this comment.
That's what I meant.. nothing required, though, just a matter of style, probably.
|
Hello @tiagobento Thanks for your patience! Please see my last commit - thanks |
|
jenkins execute fdb |
|
In case we don't have a build from the server, here's a BC I build with the latest commit https://drive.google.com/file/d/1N20VoEhCNC0vD02f_hsNXYy_98u8WCjj/view?usp=sharing |
|
Hi @jesuino, I tested starting the components and checked the rendered output on localhost:9001. All components seemed to work fine, I tried to change the data for the components and they adjusted accordingly. I have one question, though. Maybe I would figure it out but as there's no much time, I'll just ask you 😊 . What should the buttons INIT and DATASET do in practice? I can see them in the UI but IDK what I should do with it. Thanks. |
|
Hello @barboras7 Thanks for your comment, I really appreciate. These buttons (INIT and DATASET) can be used to emulate message sent from DB to the component in order to debug the component behavior. if you add a breakpoint to the component onInit or onDataset function you should be able to debug the component and modify parameters to see how it behaves. Thanks! |
|
Hello @jesuino , |
|
@jesuino I'm approving this PR since my role was to provide advice regarding TypeScript/JavaScript good practices. Since I'm not an expert on the domain of this PR, I'll let others with more knowledge verify that :) |
|
Hello @sthundat Thank you so much for your review.
In any case, we don't have the control over the SVG, so I made two small improvements:
All these changes goes inside a specific JAR that you can replace in Business Central WAR.
|
|
If you want to see the latest version just replace dashbuilder-js-7.48.0-SNAPSHOT.jar JAR in business-central.war/WEB-INF/lib with the following: https://drive.google.com/file/d/1OFDyc2GEWxBuk6h1crQ1wpWJah2z0v3H/view?usp=sharing Thanks |
|
@jesuino , |
|
jenkins execute fdb |
|
jenkins execute cdb |
1 similar comment
|
jenkins execute cdb |
|
Hi @jesuino, I tested the components by running them on localhost. Everything seems to work as it should, I just have one question. Would it be possible to avoid seeing the dashbuilder logo in the logo component? When I run the logo component, I see the Dashbuilder logo first (just for a moment) and then the Red Hat logo appears. This is not preventing the PR from merging, just letting you know, maybe this could be fixed in future. |
|
Hello @barboras7 Thanks for the suggestion, I removed the sample logo, and I had to increase the API version to publish on NPM. |
|
The previous test failures were not related: -- |
|
jenkins execute fdb |
|
SonarCloud Quality Gate failed. |
|
failed tests are not related |
ederign
left a comment
ederign
left a comment
There was a problem hiding this comment.
Great work as always William!
|
Thanks @ederign this was my 400th PR merged PR :) |
* AF-2700: Create Heatmap internal component (kiegroup#1080) * AF-2700: Create Heatmap internal component * Only used external components are exported * Review changes * Adding test for listAllComponents * Removing static functions and using a class for component Api * improving process selector * Making process selector expanded again * increasing version and removing sample logo * Clean up uberfire-workbench * Clean up uberfire-workbench * Clean up uberfire-workbench * Clean up uberfire-workbench * Clean up uberfire-workbench * Clean up uberfire-workbench * Clean up uberfire-workbench * Clean up uberfire-workbench * Clean up uberfire-workbench* * Clean up uberfire-workbench * Clean up uberfire-workbench * Clean up uberfire-workbench * Clean up uberfire-workbench * Clean up uberfire-workbench * Clean up uberfire-workbench * Clean up uberfire-workbench * Clean up uberfire-workbench * Clean up uberfire-workbench * Clean up uberfire-workbench * Clean up uberfire-workbench Co-authored-by: William Antônio Siqueira <william.fatecsjc@gmail.com>
6 participants
AF-2700: Create Heatmap internal component
This PR introduces two new provided components:
There were also some other small changes and improvements:
With the addition of categories for provided components we then moved the heatmap components to Heatmaps category.
Running Heatmap components
The new components were implemented using React/TS and live inside dashbuilder-js project. During the maven build the components artifacts are placed in a JAR which is later retrieved by components servlet.
It is possible to run specific component following the steps below:
appformer/dashbuilder/dashbuilder-shared/dashbuilder-jsyarn run init && yarn run build:fastyarn run start. Existing components are inpackagesdirectory:heatmap-component is not exported- in BC/DB you will see only log, and the two processes heatmaps components
localhost:9001staticdirectory you will find amanifest.dev.jsonfile. This file contains mocked parameters, functions and dataset that will be used to render the component - modify this file and webpack will reload the component with your changes;Usage
Components works with a dataset, so you must connect to Kie Server to retrieve actual information. I will put the steps here, but later you can import a sample dashboard that I will also include
Process Heatmap
In BC go to Pages, create a new page, find the Heatmaps category and drag the heatmap component to the page. FIrst thing you should see is a warning saying that some mandatory properties were not provided:
To solve this you go to Component Editor and set all the required properties. After doing this you should be able to see the process diagram:
Notice we have no heats - that's because we didn't provide heat values for the process nodes. In fact, component would show an error message, however, the default dataset (countries and population) match the criteria for a valid dataset - but usually users will see a message like this:
To add data create a new remote dataset using the following SQL query:
This big query contains node for all processes and multiple information about the node. In the heatmap component you must select nid column and some of the numeric columns, which are: total_hits, averageExecutionTime minExecutionTime and maxExecutionTime. Remember also to create some sample process instances or nothing will be displayed.
Processes Heatmaps
This component can show multiple processes heatmaps. It only requires the server template and the dataset must provide all containers and processes information:
Users can hide the status that shows at the bottom of the component - but the process selector will always be visible, at least it can be collapsed:
Dashbuilder Runtime
Once your components are ready you can export the ZIP that makes sue of it and run on Dashbuilder Runtime. The provided component won't be inside the ZIP because it is packaged on server side, making the ZIP small.
Sample Dashboards
The dataset and the pages I used in this PR are attached. You can import it in your installation and then go to the page test and change the configuration according to your installation or open the ZIP and modify the dataset directly, here are the steps:
Import the attached ZIP; heatmap_basic_demo.zip
Go to Datasets and edit the dataset
NODES INFORMATIONto set the correct server template. Notice that due a known issue with Remote Dataset in BC - when you edit you may see an error when testing the server. If you see it, modify the server template in Configuration tab, save the dataset and reopen it.Now go to test page and edit the two heatmaps I placed in the page to match your installation configuration (server template, process id and container id);
Everything should work after this.
Tests
I was not able to test the components directly due this bug in heatmapjs - the API we use to build the heatmap: pa7/heatmap.js#331
In summay I had to use an actual canvas in server side (node implementation) to mount the component and ensure the correct heatmap was generated, but unfortunately I was not able to do that due the bug mentioned above.
If you have any suggestion for testing please let me know in the review.