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

changed PresetTextWrap to PresetTextWarp #1448

Closed
wants to merge 6 commits into from
Closed

changed PresetTextWrap to PresetTextWarp #1448

wants to merge 6 commits into from

Conversation

tomjebo
Copy link
Collaborator

@tomjebo tomjebo commented Jun 13, 2023

No description provided.

Copy link
Collaborator

@mikeebowen mikeebowen left a comment

Choose a reason for hiding this comment

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

I think this was generated without the fix for #1444, which is needed before merging.

@@ -5023,7 +5023,7 @@
},
{
"QName": ":custScaleY",
"PropertyName": "HeightScale",
"PropertyName": "HightScale",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this undoing the fix from #1444 ?

@@ -4542,7 +4542,7 @@ public static class NoNamespace
/// <para>As an XML attribute, it:</para>
/// <list type="bullet">
/// <item><description>is contained in the following XML elements: <see cref="DGM.prSet" />.</description></item>
/// <item><description>corresponds to the following strongly-typed properties: PropertySet.HeightScale.</description></item>
/// <item><description>corresponds to the following strongly-typed properties: PropertySet.HightScale.</description></item>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question about #1444

@@ -6910,7 +6910,7 @@ public PropertySet(string outerXml) : base(outerXml)
/// <para>Height Scale</para>
/// <para>Represents the following attribute in the schema: custScaleY</para>
/// </summary>
public Int32Value? HeightScale
public Int32Value? HightScale
Copy link
Collaborator

Choose a reason for hiding this comment

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

conflict with fix for #1444

@@ -7020,7 +7020,7 @@ internal override void ConfigureMetadata(ElementMetadata.Builder builder)
.AddAttribute("custSzX", a => a.FixedWidthOverride)
.AddAttribute("custSzY", a => a.FixedHeightOverride)
.AddAttribute("custScaleX", a => a.WidthScale)
.AddAttribute("custScaleY", a => a.HeightScale)
.AddAttribute("custScaleY", a => a.HightScale)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Conflict with #1444

@tomjebo
Copy link
Collaborator Author

tomjebo commented Jun 14, 2023

I think this was generated without the fix for #1444, which is needed before merging.

yup, you're right. It's because 1444 didn't get merged on the backend. I'll take care of that and it will fix this when I remerge everything.

Also, this doesn't get caught by the tests, otherwise, it would not have made it this far. Maybe we should look into a test that covers more of the API surface to catch stuff like this.

@tomjebo
Copy link
Collaborator Author

tomjebo commented Jun 14, 2023

@mikeebowen Ok, HeightScale fix is now included.

@tomjebo tomjebo requested a review from mikeebowen June 14, 2023 04:24
mikeebowen
mikeebowen previously approved these changes Jun 14, 2023
@tomjebo
Copy link
Collaborator Author

tomjebo commented Jun 14, 2023

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

twsouthwick
twsouthwick previously approved these changes Jun 23, 2023
Copy link
Member

@twsouthwick twsouthwick left a comment

Choose a reason for hiding this comment

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

Lgtm - is there a spell check or something we could run over these property names to get them fixed before v3?

@tomjebo
Copy link
Collaborator Author

tomjebo commented Jun 26, 2023

Lgtm - is there a spell check or something we could run over these property names to get them fixed before v3?

yeah, we discussed this and we should do it anyway, but the problem is that things like "warp" to "wrap" don't trigger spell checkers. I'll give it a scan anyway.

@tomjebo tomjebo dismissed stale reviews from twsouthwick and mikeebowen via 5fff7f7 June 27, 2023 01:02
@tomjebo
Copy link
Collaborator Author

tomjebo commented Jun 27, 2023

I did a spell check scan on the front end code (non-generated) and found mostly comment spelling issues with only a couple code issues.

twsouthwick
twsouthwick previously approved these changes Jun 27, 2023
Copy link
Member

@twsouthwick twsouthwick left a comment

Choose a reason for hiding this comment

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

LGTM - although there's a merge conflict

@twsouthwick
Copy link
Member

@tomjebo you'll need move the branch to the dotnet fork instead of yours until you get the GH actions working

@tomjebo
Copy link
Collaborator Author

tomjebo commented Jun 27, 2023

@tomjebo you'll need move the branch to the dotnet fork instead of yours until you get the GH actions working

Ah, ok. I'll move this over to a new dotnet branch.

@twsouthwick
Copy link
Member

@tomjebo can this be closed?

@tomjebo tomjebo closed this Jun 29, 2023
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