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

Add support for Predefined List in Code Generation extension #23

Conversation

devlife
Copy link
Contributor

@devlife devlife commented Mar 5, 2021

Problem

When using the Code Generation extension, if a TextField with an editor of Predefined List outputs the following text

FIX ME! Couldn't determine the actual type to instantiate

However, the source code indicates that we should use type OrchardCore.ContentFields.Settings.ListValueOption

Resolution

When a JObject is passed into ConvertJToken we return a string using the name/value pair as the Name and Value properties respectively.

Screenshot

image

@Piedone
Copy link
Member

Piedone commented Mar 5, 2021

Nice, thank you!

devlife and others added 2 commits March 8, 2021 09:29
Co-authored-by: Zoltán Lehóczky <[email protected]>
@devlife
Copy link
Contributor Author

devlife commented Apr 8, 2021

Added some smarter formatting to handle simple arrays. Let me know if I missed anything else.

Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

Thanks, almost perfect!

Extensions/CodeGeneration/CodeGenerationDisplayDriver.cs Outdated Show resolved Hide resolved
Extensions/CodeGeneration/CodeGenerationDisplayDriver.cs Outdated Show resolved Hide resolved
Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

Thank you!

Extensions/CodeGeneration/CodeGenerationDisplayDriver.cs Outdated Show resolved Hide resolved
Extensions/CodeGeneration/CodeGenerationDisplayDriver.cs Outdated Show resolved Hide resolved
Extensions/CodeGeneration/CodeGenerationDisplayDriver.cs Outdated Show resolved Hide resolved
NuGet.config Outdated Show resolved Hide resolved
@Piedone
Copy link
Member

Piedone commented Sep 5, 2021

Any news on this in the meantime? There's very little missing.

@devlife
Copy link
Contributor Author

devlife commented Sep 7, 2021

Sorry I let this slip. I'll wrap it up this week.

@Piedone
Copy link
Member

Piedone commented Feb 16, 2022

How about now? :)

@devlife
Copy link
Contributor Author

devlife commented Mar 5, 2022

@Piedone I made the requested updates. The changed file list looks crazy although I only changed a single file. Do you want me to close this and create a new PR?

@Piedone Piedone changed the base branch from orchard-core-preview to dev March 6, 2022 22:34
@Piedone
Copy link
Member

Piedone commented Mar 6, 2022

The target branch should now be dev; I've retargeted the PR and it looks good now, but please check. Also, please address #23 (comment).

@devlife
Copy link
Contributor Author

devlife commented Mar 7, 2022

@Piedone Updated to use ContainsOrdinalIgnoreCase()

@Piedone
Copy link
Member

Piedone commented Mar 8, 2022

I mean, when #23 (comment) is also done ;).

@devlife
Copy link
Contributor Author

devlife commented Mar 8, 2022

@Piedone that change is already done unless I'm missing something.

_contentDefinitionManager.AlterPartDefinition("Test", part => part
    .WithField("Predefined", field => field
        .OfType("TextField")
        .WithDisplayName("Predefined")
        .WithEditor("PredefinedList")
        .WithPosition("0")
        .WithSettings(new TextFieldPredefinedListEditorSettings
        {
            Options = new[]
            {
                new ListValueOption
                {
                    Name = "One",
                    Value = "1",
                }
                new ListValueOption
                {
                    Name = "Two",
                    Value = "2",
                }
                new ListValueOption
                {
                    Name = "Three",
                    Value = "3",
                }
            },
            DefaultValue = "1",
        })
    )
    .WithField("Picker", field => field
        .OfType("ContentPickerField")
        .WithDisplayName("Picker")
        .WithPosition("1")
        .WithSettings(new ContentPickerFieldSettings
        {
            DisplayedContentTypes = new[]
            {
                "Article",
                "ContentMenuItem",
                "Paragraph",
            },
        })
    )
);

@Piedone
Copy link
Member

Piedone commented Mar 8, 2022

I commented before your commit :). I'll check it out.

@devlife devlife requested a review from Piedone March 8, 2022 19:31
Extensions/CodeGeneration/CodeGenerationDisplayDriver.cs Outdated Show resolved Hide resolved
Extensions/CodeGeneration/CodeGenerationDisplayDriver.cs Outdated Show resolved Hide resolved
Extensions/CodeGeneration/CodeGenerationDisplayDriver.cs Outdated Show resolved Hide resolved
case JArray jArray:
return $"new[] {{ {string.Join(", ", jArray.Select(ConvertJToken))} }}";
var format = $"{string.Join(string.Empty, Enumerable.Repeat(" ", indentationDepth + 4))}";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var format = $"{string.Join(string.Empty, Enumerable.Repeat(" ", indentationDepth + 4))}";
var indentation = $"{string.Join(string.Empty, Enumerable.Repeat(" ", indentationDepth + 4))}";

Copy link
Contributor

@0liver 0liver Mar 8, 2022

Choose a reason for hiding this comment

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

You can simplify this by using this string constructor:

Suggested change
var format = $"{string.Join(string.Empty, Enumerable.Repeat(" ", indentationDepth + 4))}";
var indentation = new string(' ', indentationDepth + 4);

The same goes for the other occurence.

Copy link
Contributor

Choose a reason for hiding this comment

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

@devlife Did you see these suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah no I did not. I replaced the string.Join statements with the string ctor.

Extensions/CodeGeneration/CodeGenerationDisplayDriver.cs Outdated Show resolved Hide resolved
@devlife
Copy link
Contributor Author

devlife commented Mar 9, 2022

Hey guys. Any other changes you'd like to see?

@Piedone
Copy link
Member

Piedone commented Mar 9, 2022

There are build errors, and #23 (comment) (the variable name) is not addressed.

@devlife
Copy link
Contributor Author

devlife commented Mar 9, 2022

I updated the variable name but it compiles without any issues for me. Can you point me to the build issues you're seeing?

@Piedone
Copy link
Member

Piedone commented Mar 9, 2022

Well, now it compiles with your new commits ;). #23 (comment) is not addressed though.

@devlife
Copy link
Contributor Author

devlife commented Mar 9, 2022

comment 23 is addressed!

@Piedone
Copy link
Member

Piedone commented Mar 9, 2022

Thanks for fixing the other occasions too!

@Piedone Piedone merged commit 691b966 into Lombiq:dev Mar 9, 2022
@Piedone
Copy link
Member

Piedone commented Mar 9, 2022

And it's released, also on NuGet: https://github.com/Lombiq/Helpful-Extensions/releases/tag/v2.1.0

@devlife
Copy link
Contributor Author

devlife commented Mar 9, 2022

Woohoo! That's awesome thanks so much for your help @Piedone @0liver

@devlife devlife deleted the extension/code-generation/add-predefined-list-handling branch March 9, 2022 19:32
@0liver
Copy link
Contributor

0liver commented Mar 9, 2022

Thank YOU for your long wind!

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.

3 participants