Skip to content
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

draft decisionPropertiesPanel.spec.ts #26

Conversation

jomarko
Copy link
Collaborator

@jomarko jomarko commented Mar 20, 2024

No description provided.

Copy link
Owner

@ljmotta ljmotta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a few comments inline. But the major change would be to add the missing classes and tests, and to use them instead of the generalProperties on the tests.

For example on the bkm tests.
await bkmPropertiesPanel.setName({ ... }) instead of await generalProperties.changeNodeName({ ... })

@@ -375,6 +374,7 @@ export function FontOptions({ startExpanded, nodeIds }: { startExpanded: boolean
</div>
<br />
<Select
ouiaId="node-font-style-selector"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is used in current changeNodeFont and getNodeFont

  public async changeNodeFont(args: { nodeName: string; newFont: string }) {
    await this.selectNodeToLoadPropertiesPanel({ nodeName: args.nodeName });
    await this.panel().getByTitle("Expand / collapse Font").click();

    await this.panel().locator("[data-ouia-component-id='node-font-style-selector']").click();
    await this.panel().getByText(args.newFont).click();

    await this.diagram.resetFocus();
  }

  public async getNodeFont(args: { nodeName: string }) {
    await this.selectNodeToLoadPropertiesPanel({ nodeName: args.nodeName });
    await this.panel().getByTitle("Expand / collapse Font").click();

    return await this.panel().locator("[data-ouia-component-id='node-font-style-selector']").textContent();
  }

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok, it seens I've missed this. We don't use ouiaId on any other place. Please use a data-* instead. The ouiaId is a Patternfly prop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorporated as part of #27

Comment on lines +40 to +44
generalProperties: GeneralProperties;
decisionProperties: DecisionProperties;
knowledgeSourceProperties: KnowledgeSourceProperties;
propertiesPanel: PropertiesPanelBase;
generalDecisionServiceProperties: GeneralDecisionServiceProperties;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this, I don't think a consistency between the names.

Suggested change
generalProperties: GeneralProperties;
decisionProperties: DecisionProperties;
knowledgeSourceProperties: KnowledgeSourceProperties;
propertiesPanel: PropertiesPanelBase;
generalDecisionServiceProperties: GeneralDecisionServiceProperties;
generalPropertiesPanel: GeneralPropertiesPanel;
decisionPropertiesPanel: DecisionPropertiesPanel;
knowledgeSourcePropertiesPanel: KnowledgeSourcePropertiesPanel;
propertiesPanel: PropertiesPanel;
decisionServicePropertiesPanel: DecisionServicePropertiesPanel;

Also, what is generalProperties? If we want to use this class as a base for the others, we still need to create the specific ones. After we have the specific, we wouldn't need to directly use the "GeneralProperties" class.

From this, I can see it's missing a few panels:

  • TextAnnotation
  • Group
  • InputData
  • BKM
  • Global (when no node is selected)
  • Multi (when multiple nodes are selected)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorporated as part of #27

import { PropertiesPanelBase } from "./propertiesPanelBase";

export class DecisionProperties extends PropertiesPanelBase {
public async changeNodeQuestion(args: { nodeName: string; newQuestion: string }) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the panel is from the Node, we don't need to add it to the name. And as we're using the get, we could use set. This way we would have setQuestion. The same applies to the other methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorporated as part of #27

import { GeneralProperties } from "./generalProperties";

export class GeneralDecisionServiceProperties extends GeneralProperties {
public async selectNodeToLoadPropertiesPanel(args: { nodeName: string }) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we need this at all. The select node is already on the node fixture, as you have used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well this was an attempt to minimize code repetition and consequence of class hierarchy we have, I need to rethink more during #27 completion.

import { DataType } from "../jsonModel";
import { PropertiesPanelBase } from "./propertiesPanelBase";

export class GeneralProperties extends PropertiesPanelBase {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CommonPropertiesPanel?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of date, see #27

Comment on lines +31 to +33
public async selectNodeToLoadPropertiesPanel(args: { nodeName: string }) {
await this.nodes.select({ name: args.nodeName });
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned before, we don't need this. Selecting the node, from the nodes fixture.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well this was an attempt to minimize code repetition and consequence of class hierarchy we have, I need to rethink more during #27 completion.

});

test.skip("should change the BKM node shape - background color", async ({ nodes, propertiesPanel }) => {
// blocked https://github.com/microsoft/playwright/issues/19929#issuecomment-1377035969
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue is related to Firefox, do we have the same problem on chromium and webkit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to recheck as part of #27

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changeBkmProperties

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in #27

test("should change the BKM node name", async ({ nodes, generalProperties }) => {
await generalProperties.changeNodeName({ from: DefaultNodeName.BKM, to: "Renamed BKM" });

await expect(nodes.get({ name: "Renamed BKM" })).toBeVisible();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make an assertion on the properties panel too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in #27

@jomarko
Copy link
Collaborator Author

jomarko commented Apr 2, 2024

Closing this PR in favor of #27 because both PRs are too interconnected and split if incorporating the feedback to both #26 and #27 would make the work difficult.

@jomarko jomarko closed this Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants