You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "yesamer (via GitHub)" <gi...@apache.org> on 2024/01/04 11:37:19 UTC
[PR] kie-issues#737: Background Tab [incubator-kie-tools]
yesamer opened a new pull request, #2108:
URL: https://github.com/apache/incubator-kie-tools/pull/2108
Closes https://github.com/apache/incubator-kie-issues/issues/737
In progress...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] kie-issues#737: Test Scenario Editor: Background Tab [incubator-kie-tools]
Posted by "jomarko (via GitHub)" <gi...@apache.org>.
jomarko commented on code in PR #2108:
URL: https://github.com/apache/incubator-kie-tools/pull/2108#discussion_r1442607281
##########
packages/scesim-editor/src/table/TestScenarioTable.tsx:
##########
@@ -81,11 +88,48 @@ function TestScenarioTable({
const { i18n } = useTestScenarioEditorI18n();
- const tableScrollableElementRef = useRef<{ current: HTMLDivElement | null }>({ current: null });
+ /** BACKGROUND TABLE MANAGMENT */
- useEffect(() => {
- tableScrollableElementRef.current.current = document.querySelector(".kie-scesim-editor--table-container") ?? null;
- }, []);
+ const isBackground = useMemo(() => {
+ return "BackgroundData" in tableData.scesimData;
+ }, [tableData]);
+
+ const columnIndexStart = useMemo(() => {
+ return isBackground ? 0 : 1;
+ }, [isBackground]);
+
+ const retrieveRowsData = useCallback(
+ (rowData: SceSim__backgroundDatasType | SceSim__scenariosType) => {
+ if (isBackground) {
+ return (rowData as SceSim__backgroundDatasType).BackgroundData;
+ } else {
+ return (rowData as SceSim__scenariosType).Scenario;
+ }
+ },
+ [isBackground]
+ );
Review Comment:
@yesamer thank you for the PR. Just out of interest, could you please explain the need of both `SceSim__backgroundDatasType` and `SceSim__scenariosType`?
They seems to be the same in xsd:
```
<xsd:complexType name="ScenarioType">
<xsd:sequence>
<xsd:element type="factMappingValuesType" name="factMappingValues" />
</xsd:sequence>
</xsd:complexType>
<xsd:complexType name="BackgroundDataType">
<xsd:sequence>
<xsd:element type="factMappingValuesType" name="factMappingValues" />
</xsd:sequence>
</xsd:complexType>
<xsd:complexType name="scenariosType">
<xsd:sequence>
<xsd:element type="ScenarioType" name="Scenario" maxOccurs="unbounded" minOccurs="0" />
</xsd:sequence>
</xsd:complexType>
<xsd:complexType name="backgroundDatasType">
<xsd:sequence>
<xsd:element type="BackgroundDataType" name="BackgroundData" maxOccurs="unbounded"
minOccurs="0" />
</xsd:sequence>
```
And most of the code (pseudocode) seems to use `isbackground` that is implemented without the need of `SceSim__backgroundDatasType` or `SceSim__scenariosType`.
I mean we do most the time:
```
if (isBackground) {
// do something
}
```
We do not do:
```
if (is SceSim__backgroundDatasType) {do sth}
else if (is SceSim__scenariosType) {do sth else}
```
Just trying to understand code better. I am not saying the current version is wrong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] kie-issues#737: Test Scenario Editor: Background Tab [incubator-kie-tools]
Posted by "tiagobento (via GitHub)" <gi...@apache.org>.
tiagobento merged PR #2108:
URL: https://github.com/apache/incubator-kie-tools/pull/2108
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] kie-issues#737: Test Scenario Editor: Background Tab [incubator-kie-tools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #2108:
URL: https://github.com/apache/incubator-kie-tools/pull/2108#discussion_r1453154719
##########
packages/scesim-editor/src/table/TestScenarioTable.tsx:
##########
@@ -81,11 +88,48 @@ function TestScenarioTable({
const { i18n } = useTestScenarioEditorI18n();
- const tableScrollableElementRef = useRef<{ current: HTMLDivElement | null }>({ current: null });
+ /** BACKGROUND TABLE MANAGMENT */
- useEffect(() => {
- tableScrollableElementRef.current.current = document.querySelector(".kie-scesim-editor--table-container") ?? null;
- }, []);
+ const isBackground = useMemo(() => {
+ return "BackgroundData" in tableData.scesimData;
+ }, [tableData]);
+
+ const columnIndexStart = useMemo(() => {
+ return isBackground ? 0 : 1;
+ }, [isBackground]);
+
+ const retrieveRowsData = useCallback(
+ (rowData: SceSim__backgroundDatasType | SceSim__scenariosType) => {
+ if (isBackground) {
+ return (rowData as SceSim__backgroundDatasType).BackgroundData;
+ } else {
+ return (rowData as SceSim__scenariosType).Scenario;
+ }
+ },
+ [isBackground]
+ );
Review Comment:
Hi @jomarko. apologies for my late reply. You highlighted a very interesting point, I'll investigate more and try to check if your comment is applicable. Thx!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] kie-issues#737: Test Scenario Editor: Background Tab [incubator-kie-tools]
Posted by "jomarko (via GitHub)" <gi...@apache.org>.
jomarko commented on code in PR #2108:
URL: https://github.com/apache/incubator-kie-tools/pull/2108#discussion_r1442607281
##########
packages/scesim-editor/src/table/TestScenarioTable.tsx:
##########
@@ -81,11 +88,48 @@ function TestScenarioTable({
const { i18n } = useTestScenarioEditorI18n();
- const tableScrollableElementRef = useRef<{ current: HTMLDivElement | null }>({ current: null });
+ /** BACKGROUND TABLE MANAGMENT */
- useEffect(() => {
- tableScrollableElementRef.current.current = document.querySelector(".kie-scesim-editor--table-container") ?? null;
- }, []);
+ const isBackground = useMemo(() => {
+ return "BackgroundData" in tableData.scesimData;
+ }, [tableData]);
+
+ const columnIndexStart = useMemo(() => {
+ return isBackground ? 0 : 1;
+ }, [isBackground]);
+
+ const retrieveRowsData = useCallback(
+ (rowData: SceSim__backgroundDatasType | SceSim__scenariosType) => {
+ if (isBackground) {
+ return (rowData as SceSim__backgroundDatasType).BackgroundData;
+ } else {
+ return (rowData as SceSim__scenariosType).Scenario;
+ }
+ },
+ [isBackground]
+ );
Review Comment:
@yesamer thank you for the PR. Just out of interest, could you please explain the need of both `SceSim__backgroundDatasType` and `SceSim__scenariosType`?
They seems to be the same in xsd:
```
<xsd:complexType name="ScenarioType">
<xsd:sequence>
<xsd:element type="factMappingValuesType" name="factMappingValues" />
</xsd:sequence>
</xsd:complexType>
<xsd:complexType name="BackgroundDataType">
<xsd:sequence>
<xsd:element type="factMappingValuesType" name="factMappingValues" />
</xsd:sequence>
</xsd:complexType>
<xsd:complexType name="scenariosType">
<xsd:sequence>
<xsd:element type="ScenarioType" name="Scenario" maxOccurs="unbounded" minOccurs="0" />
</xsd:sequence>
</xsd:complexType>
<xsd:complexType name="backgroundDatasType">
<xsd:sequence>
<xsd:element type="BackgroundDataType" name="BackgroundData" maxOccurs="unbounded"
minOccurs="0" />
</xsd:sequence>
```
And most of the code (pseudocode) seems to use `isbackground` that is implemented without the need of `SceSim__backgroundDatasType` or `SceSim__scenariosType`.
I mean we do most the time:
```
if (isBackground) {
// do something
}
```
We do not do:
```
if (is SceSim__backgroundDatasType) {do sth}
else if (is SceSim__scenariosType) {do sth else}
```
Just trying to understand code better. I am not saying the current version is wrong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] kie-issues#737: Test Scenario Editor: Background Tab [incubator-kie-tools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on PR #2108:
URL: https://github.com/apache/incubator-kie-tools/pull/2108#issuecomment-1893427679
@jomarko I applied the following change:
<img width="823" alt="Screenshot 2024-01-16 at 10 51 03" src="https://github.com/apache/incubator-kie-tools/assets/16005046/71a23882-ae14-41b5-9aad-67e70a541986">
Here, I merged `ScenarioType` and `BackgroundDataType` types on a single new type FactMappingValuesTypes, considering they hold the data as you correctly spotted.
Can't do the same for`scenariosType` & `backgroundDatasType` types: they hold the same data, but they are bound to different tag names, and that means I can't remove those `if` management. Does make sense? WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] kie-issues#737: Test Scenario Editor: Background Tab [incubator-kie-tools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on PR #2108:
URL: https://github.com/apache/incubator-kie-tools/pull/2108#issuecomment-1877495630
@tiagobento Ready to be reviewed / merged :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org