ljmotta commented on code in PR #2188: URL: https://github.com/apache/incubator-kie-tools/pull/2188#discussion_r1522069613
########## packages/scesim-editor/stories/dev/DevWebApp.stories.tsx: ########## @@ -0,0 +1,74 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import type { Meta, StoryObj } from "@storybook/react"; +import { SceSimEditorWrapper } from "../scesimEditorStoriesWrapper"; +import { Button, Flex, FlexItem, Title, Tooltip } from "@patternfly/react-core/dist/js"; +import React, { useEffect, useState } from "react"; +import { linkTo } from "@storybook/addon-links"; + +function App() { + const [version, setVersion] = useState(-1); + + useEffect(() => { + setVersion((prev) => prev + 1); + }, []); + + return ( + <div> + <Flex direction={{ default: "column" }}> + <FlexItem> + <Flex style={{ width: "96vw" }}> + <FlexItem> + <Button onClick={linkTo("Misc/Empty SceSim Editor", "Base")}>Empty</Button> + </FlexItem> + <FlexItem> + <Button onClick={linkTo("Use Cases/Is Old Enough Rule", "Is Old Enough")}>Are They Old Enough?</Button> + </FlexItem> + <FlexItem> + <Button onClick={linkTo("Use Cases/Traffic Violation DMN", "Traffic Violation")}> + Traffic Violation + </Button> + </FlexItem> + <FlexItem align={{ default: "alignRight" }}> + <Tooltip content={"This number updates everytime the expressionDefinition object is updated"}> + <Title headingLevel="h2">Updates count: {version}</Title> + </Tooltip> + </FlexItem> + </Flex> + </FlexItem> + <FlexItem> + <div>{SceSimEditorWrapper()}</div> + </FlexItem> + </Flex> + </div> + ); +} +const meta: Meta<typeof App> = { + title: "Dev/Web App", + component: App, +}; + +export default meta; +type Story = StoryObj<typeof App>; + +export const WebApp: Story = { + render: (args) => App(), + args: {}, Review Comment: It still require to add the `args` so it can appear on the Storybook properties panel. ########## packages/scesim-editor/stories/scesimEditorStoriesWrapper.tsx: ########## @@ -0,0 +1,31 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import * as React from "react"; +import { useRef } from "react"; +import { DevWebApp } from "../dev-webapp/src/DevWebApp"; + +export function SceSimEditorWrapper() { Review Comment: After you add the `args` you will notice that changing the `args` will not reflect on the App, this is because the wrapper requires a few additional things to make it work. ########## packages/scesim-editor/tests/e2e/__fixtures__/cells.ts: ########## @@ -0,0 +1,37 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { Page, Locator } from "@playwright/test"; + +export class Cells { + constructor(public page: Page) {} + + public async navigateLeft(target: Locator) { + await target.press("ArrowLeft"); + } + public async navigateRight(target: Locator) { + await target.press("ArrowRight"); + } + public async navigateUp(target: Locator) { + await target.press("ArrowUp"); + } + public async navigateDown(target: Locator) { + await target.press("ArrowDown"); + } Review Comment: We could pass a way to find the cell instead of the cell itself. For example: ``` await page.getByRole("row", { name: args.rowNumber, exact: true }).getByTestId("monaco-container").nth(args.columnNumber) ``` Looking at it after this change, this methods doens't make much sense into the `Cell` fixture, as a `Cell` doens't know about `row` and ` column`. I guess `TestScenarioTable` would make more sense or `Table` as it should work on both? WDYT? ########## packages/scesim-editor/stories/scesimEditorStoriesWrapper.tsx: ########## @@ -0,0 +1,31 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import * as React from "react"; +import { useRef } from "react"; +import { DevWebApp } from "../dev-webapp/src/DevWebApp"; + +export function SceSimEditorWrapper() { + const emptyRef = useRef<HTMLDivElement>(null); + return ( + <div ref={emptyRef}> + <DevWebApp /> Review Comment: We shouldn't use anymore the previous `DevWebApp`. Copy it to the `DevWebApp.stories.tsx`. Here you should render the `<TestScenarioEditor />` ########## packages/scesim-editor/stories/dev/DevWebApp.stories.tsx: ########## @@ -0,0 +1,74 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import type { Meta, StoryObj } from "@storybook/react"; +import { SceSimEditorWrapper } from "../scesimEditorStoriesWrapper"; +import { Button, Flex, FlexItem, Title, Tooltip } from "@patternfly/react-core/dist/js"; +import React, { useEffect, useState } from "react"; +import { linkTo } from "@storybook/addon-links"; + +function App() { + const [version, setVersion] = useState(-1); + + useEffect(() => { + setVersion((prev) => prev + 1); + }, []); + + return ( + <div> + <Flex direction={{ default: "column" }}> + <FlexItem> + <Flex style={{ width: "96vw" }}> + <FlexItem> + <Button onClick={linkTo("Misc/Empty SceSim Editor", "Base")}>Empty</Button> + </FlexItem> + <FlexItem> + <Button onClick={linkTo("Use Cases/Is Old Enough Rule", "Is Old Enough")}>Are They Old Enough?</Button> + </FlexItem> + <FlexItem> + <Button onClick={linkTo("Use Cases/Traffic Violation DMN", "Traffic Violation")}> + Traffic Violation Review Comment: I'm not sure how this works. By performing a `linkTo` we will not leave the Dev webapp? ########## packages/scesim-editor/stories/useCases/IsOldEnoughRule.ts: ########## Review Comment: Inline it on the `IsOldEnoughRule.stories.tsx`. ########## packages/scesim-editor/stories/useCases/IsOldEnoughRule.stories.tsx: ########## @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import type { Meta, StoryObj } from "@storybook/react"; +import { TestScenarioEditor, TestScenarioEditorRef } from "../../src/TestScenarioEditor"; +import { useEffect, useRef } from "react"; +import React from "react"; +import { IS_OLD_ENOUGH_RULE } from "./IsOldEnoughRule"; + +function IsOldEnoughRule() { + const ref = useRef<TestScenarioEditorRef>(null); + useEffect(() => { + ref.current?.setContent("AreTheyOldEnoughTest.scesim", IS_OLD_ENOUGH_RULE); + }, []); + return <TestScenarioEditor ref={ref} />; +} +const meta: Meta<typeof IsOldEnoughRule> = { + title: "Use Cases/Is Old Enough Rule", + component: IsOldEnoughRule, +}; + +export default meta; +type Story = StoryObj<typeof IsOldEnoughRule>; + +export const IsOldEnough: Story = { + render: (args) => IsOldEnoughRule(), + args: {}, Review Comment: The same as the Dev webapp and Empty story ########## packages/scesim-editor/stories/misc/Empty/EmptyEditor.stories.tsx: ########## @@ -0,0 +1,36 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import type { Meta, StoryObj } from "@storybook/react"; +import { SceSimEditorWrapper } from "../../scesimEditorStoriesWrapper"; +import { TestScenarioEditor } from "../../../src/TestScenarioEditor"; + +// More on how to set up stories at: https://storybook.js.org/docs/react/writing-stories/introduction#default-export +const meta: Meta<typeof TestScenarioEditor> = { + title: "Misc/Empty SceSim Editor", + component: TestScenarioEditor, + includeStories: /^[A-Z]/, +}; +export default meta; +type Story = StoryObj<typeof TestScenarioEditor>; + +export const Base: Story = { + render: (args) => SceSimEditorWrapper(), + args: {}, Review Comment: The same as the Dev Webapp ########## packages/scesim-editor/stories/useCases/TrafficViolationDmn.stories.tsx: ########## Review Comment: The same as the `IsOldEnoughRule` ########## packages/scesim-editor/stories/useCases/IsOldEnoughRule.stories.tsx: ########## @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import type { Meta, StoryObj } from "@storybook/react"; +import { TestScenarioEditor, TestScenarioEditorRef } from "../../src/TestScenarioEditor"; +import { useEffect, useRef } from "react"; +import React from "react"; +import { IS_OLD_ENOUGH_RULE } from "./IsOldEnoughRule"; Review Comment: Please, add the `IS_OLD_ENOUGH_RULE` on this file. If this constant is used on other files, you need to add a filter to the `meta` constant: `includeStories: /^[A-Z]/`. Storybook renders everything that is exported on the `.stories.tsx` file, and this filter will remove all lowered case constants. ########## packages/scesim-editor/stories/useCases/IsOldEnoughRule.stories.tsx: ########## @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import type { Meta, StoryObj } from "@storybook/react"; +import { TestScenarioEditor, TestScenarioEditorRef } from "../../src/TestScenarioEditor"; +import { useEffect, useRef } from "react"; +import React from "react"; +import { IS_OLD_ENOUGH_RULE } from "./IsOldEnoughRule"; + +function IsOldEnoughRule() { + const ref = useRef<TestScenarioEditorRef>(null); + useEffect(() => { + ref.current?.setContent("AreTheyOldEnoughTest.scesim", IS_OLD_ENOUGH_RULE); + }, []); + return <TestScenarioEditor ref={ref} />; Review Comment: This should be inside the wrapper, and the values `"AreTheyOldEnoughTest.scesim", IS_OLD_ENOUGH_RULE` can be passed by parameters. ########## packages/scesim-editor/tests/e2e/__fixtures__/monaco.ts: ########## @@ -0,0 +1,66 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { ProjectName } from "@kie-tools/playwright-base/projectNames"; +import { Page, Locator } from "@playwright/test"; + +export class Monaco { Review Comment: Here we have the same as the `Cell` fixture. `Monaco` shouldn't know about `row` and `column`, or which table it's in. We could add this methods two different fixtures `'TestScenarioTable` and `BackgroudTable`. ########## packages/scesim-editor/stories/useCases/TrafficViolationDmn.ts: ########## Review Comment: Please inline it on the `TrafficViolationDmn.stories.tsx`. ########## packages/scesim-editor/tests/e2e/__fixtures__/scesimEditor.ts: ########## @@ -0,0 +1,57 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { Page, Locator } from "@playwright/test"; + +export enum AddRowPosition { + ABOVE, + BELOW, +} + +export enum AddColumnPosition { + LEFT, + RIGHT, +} + +export class SceSimEditor { Review Comment: I guess it could be renamed to `Table` following the changes I proposed on the other files? The editor is related to everything, and on the `Editor` fixture I guess we need the `openEmpty` method to open the empty story, a `changeToBackgroundTable` , `changeToTestScenarioTable`, and more general stuff. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
