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

added support for namemaps on control properties #725

Closed
wants to merge 3 commits into from

Conversation

mizrael
Copy link
Contributor

@mizrael mizrael commented Oct 3, 2024

added new PropertiesNameMaps property on the ControlInstance class

Screens:
  Screen1:
    Children:
      - Dropdown1:
          Control: Classic/DropDown
          Properties:
            Items: |-
              =[{Id: 1, Title: "Item 1", Desc: "desc1"}, {Id: 2, Title: "Item 2", Desc: "desc2"}, {Id: 3, Title: "Item 3", Desc: "desc3"}]
            X: =246
            Y: =420
          PropertiesNameMaps:
            Items:
              Value: Title

@mizrael mizrael requested review from a team as code owners October 3, 2024 13:52
Copy link
Contributor

@joem-msft joem-msft left a comment

Choose a reason for hiding this comment

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

I'd hold off on getting the schema for this approved first. but my comments are relevant none theless.

src/Persistence/PaYaml/Models/SchemaV3/ControlInstance.cs Outdated Show resolved Hide resolved
Properties:
Items: |-
=[{Id: 1, Title: "Item 1", Desc: "desc1"}, {Id: 2, Title: "Item 2", Desc: "desc2"}, {Id: 3, Title: "Item 3", Desc: "desc3"}]
PropertiesNameMaps:
Copy link
Contributor

Choose a reason for hiding this comment

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

another proposal would be to just use NameMaps, we can probably elide the 'Properties' prefix. But lets wait for a review on the schema before changing the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PropertiesNameMaps makes it clear that we're referring to additional metadata to enrich existing properties

Properties:
Items: |-
=[{Id: 1, Title: "Item 1", Desc: "desc1"}, {Id: 2, Title: "Item 2", Desc: "desc2"}, {Id: 3, Title: "Item 3", Desc: "desc3"}]
PropertiesNameMaps:
Copy link
Contributor

Choose a reason for hiding this comment

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

Open this file in VS Code. You'll see there are some squigglies due to this property not existing in the schema.
Whenever changing the schema, we should update the schema file here: src\schemas\pa-yaml\v3.0\pa.schema.yaml

I can talk with you offline how best to test changes in your inner-dev-loop.

After updating that file sufficiently, then you run src\schemas\publish.cmd so it compiles and updates the final file at schemas\pa-yaml\v3.0\pa.schema.yaml.

@mizrael
Copy link
Contributor Author

mizrael commented Oct 3, 2024

abandoning this PR, we have agreed on moving forward with a different schema to represent name maps:

- MyControl
    Control: Chart
    Properties:
      Items: |-
        =[{Id: 1, Title: "Item 1", Desc: "desc1"},
          {Id: 2, Title: "Item 2", Desc: "desc2"}]
      Items.Label: =Title
      Items.Series1: =Val1
      Items.Series2: =Val2
      Items.Series3: =Val1

@mizrael mizrael closed this Oct 3, 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