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

More Sample Option Attributes #336

Merged
merged 30 commits into from
Feb 1, 2023
Merged

More Sample Option Attributes #336

merged 30 commits into from
Feb 1, 2023

Conversation

niels9001
Copy link
Collaborator

@niels9001 niels9001 commented Dec 12, 2022

Related to #207

This PR introduces the following changes:

  • Adding the TextOption (PlaceholderText can be set)
  • Adding the NumericOption (Initial, Min, Max, Step, ShowAsNumberBox can be set)
  • Replacing the RadioButtons in the MultiChoiceOption with ComboBoxes to save space.

Settings

How to test:

  • Run ProjectTemplate.sln, or:
    <TextBlock
                Text="{x:Bind TextContent, Mode=OneWay}"
                FontFamily="{x:Bind TextFontFamily, Mode=OneWay}"
                Foreground="{x:Bind TextForeground, Mode=OneWay}"
                Visibility="{x:Bind IsTextVisible, Mode=OneWay}"
                FontSize="{x:Bind TextSize, Mode=OneWay}" />
  • Add these options:
[ToolkitSampleTextOption("TextContent", "This is text", Title = "Input the text")]
[ToolkitSampleMultiChoiceOption("TextFontFamily", "Segoe UI", "Arial", "Consolas", Title = "Font family")]
[ToolkitSampleMultiChoiceOption("TextForeground", "Teal : #0ddc8c", "Sand : #e7a676", "Dull green : #5d7577", Title = "Text foreground")]
[ToolkitSampleBoolOption("IsTextVisible", true, Title = "IsVisible")]
[ToolkitSampleNumericOption("TextSize", 12, 8, 48, 2, false, Title = "FontSize")]  // To show as Slider (default)
[ToolkitSampleNumericOption("TextSize", 12, 8, 48, 2, true, Title = "FontSize")]  // To show as NumberBox

@michael-hawker
Copy link
Member

@niels9001 added link to #207 at top of your description.

FYI @Arlodotexe as well.

I believe the optional title parameter is to give it a different header vs. just using the name of the binding property. However, we have this inconsistency between putting it as the 2nd parameter or at the end as optional.

I feel like we maybe want to be more consistent here? Or should we just get rid of title all together and either just use the same name as the variable (that's going to be shown in the XAML anyway for the example code) or just auto-insert a space before each capital letter?

Thoughts?

@Arlodotexe
Copy link
Member

Arlodotexe commented Dec 12, 2022

I feel like we maybe want to be more consistent here? Or should we just get rid of title all together and either just use the same name as the variable (that's going to be shown in the XAML anyway for the example code) or just auto-insert a space before each capital letter?

@michael-hawker Custom titles should be optional (for complex or multi-control samples), though we could use the bound variable name as the default.

@michael-hawker
Copy link
Member

Custom titles should be optional (but enabled, for multi-control samples)

@Arlodotexe what do you mean? What would a 'multi-control' sample be? Like where would we need to customize this.

I think it's clearer if we have the text matching the property they're going to see bound in the XAML example code, right? Otherwise if you're looking at the sample code you don't know what the toggle/slider/etc... is manipulating.

@Arlodotexe
Copy link
Member

Arlodotexe commented Dec 12, 2022

What I mean is that there will be (currently are, in the case of Rive) cases where a label might not be needed or where a custom description provides more value than using the variable name.

We shouldn't lock ourselves out of having the option. Using the bindingName as the default title is a great idea, though.

@niels9001
Copy link
Collaborator Author

@Arlodotexe CI complains about 'Arguments missing'.. but I can't figure out where that'd be.. Do you see if I am missing something obvious :)?

@niels9001
Copy link
Collaborator Author

@Arlodotexe CI complains about 'Arguments missing'.. but I can't figure out where that'd be.. Do you see if I am missing something obvious :)?

Fixed! Thanks for the help.

@michael-hawker Added a bool to switch between NumberBox / Slider as well - see original post. Once this passes CI, this should be ready to review :)!

@niels9001
Copy link
Collaborator Author

One thing that's not working: the titles do not show up.

@michael-hawker
Copy link
Member

@niels9001 the build is failing on the tests that check/look that these attributes are parsed/working as expected, so you'll need to update/add to those as well.

As for the Title not displaying, @Arlodotexe thoughts?

@michael-hawker michael-hawker self-assigned this Jan 31, 2023
@michael-hawker
Copy link
Member

michael-hawker commented Feb 1, 2023

image

Have a fix incoming for the issue with Title not appearing.

Since we switched to using the property on the Attribute, this wasn't setup when we rehydrated the attributes to store for creating the sample registry, thus the Title was blank and not propagated to the UI.

We needed an extra step to grab the property value on the attribute in the source generator when it ran so that we could add it back into the attribute we use for storage to generate the registry from.

This would then let it flow up back the rest of the chain to the UI and display properly once fixed.

Had to also add in some extra helper to the tests for checking the generated source (we can probably do more to clean this up later), would be nice to compile the source or get the compiled instance of the source to check actual values via code vs. comparing strings. Would make it easier to call out specific things missing too vs. having to check manually if a test fails.

Generator tests are passing for me locally:

image

So, we'll have to see what happens in CI now.

…erly from construction of attribute in source generator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants