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

LT-21761: Add Table indenting and right justification #35

Merged
merged 2 commits into from
Apr 30, 2024

Conversation

mark-sil
Copy link
Contributor

@mark-sil mark-sil commented Apr 30, 2024

Change-Id: I7f9835e66c6aaa5cb9039cc586683c1cb0311138


This change is Reviewable

Change-Id: I7f9835e66c6aaa5cb9039cc586683c1cb0311138
Copy link
Contributor

@aror92 aror92 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mark-sil)


Src/xWorks/WordStylesGenerator.cs line 966 at r1 (raw file):

		/// <param name="tableAlignment">Returns the table alignment.</param>
		/// <returns>Returns the indentation value.</returns>
		internal static int GetTableIndentInfo(ReadOnlyPropertyTable propertyTable, ref TableRowAlignmentValues tableAlignment)

It might make more sense to place this function earlier in the code, above some of the helper functions. Perhaps right after "CalculateParagraphIndentsFromAncestors"? But this location works too if it seems better to you.


Src/xWorks/WordStylesGenerator.cs line 974 at r1 (raw file):

			}

			var projectStyle = styleSheet.Styles[WordStylesGenerator.DictionaryNormal];

Do we know for sure that we'll always want to base this off DictionaryNormal? For most types of dictionary content, Configure Dictionary allows the user to change the default style used for that content, or even specify a custom one. Not sure how it works with tables, though.

Use the table parent as the basis.

Change-Id: I9e49a9570c7c9d09696e1bb4c4ba22636e5cb1af
Copy link
Contributor Author

@mark-sil mark-sil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @aror92)


Src/xWorks/WordStylesGenerator.cs line 966 at r1 (raw file):

Previously, aror92 (Ariel Ror.) wrote…

It might make more sense to place this function earlier in the code, above some of the helper functions. Perhaps right after "CalculateParagraphIndentsFromAncestors"? But this location works too if it seems better to you.

I agree. Moved the function.


Src/xWorks/WordStylesGenerator.cs line 974 at r1 (raw file):

Previously, aror92 (Ariel Ror.) wrote…

Do we know for sure that we'll always want to base this off DictionaryNormal? For most types of dictionary content, Configure Dictionary allows the user to change the default style used for that content, or even specify a custom one. Not sure how it works with tables, though.

No, I shouldn't have used DictionaryNormal. Now it is based on the style of the parent.

Copy link
Contributor

@aror92 aror92 left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mark-sil)


Src/xWorks/WordStylesGenerator.cs line 974 at r1 (raw file):

Previously, mark-sil (Mark Kidder) wrote…

No, I shouldn't have used DictionaryNormal. Now it is based on the style of the parent.

One more question: do table nodes ever have a style specified, or will config.Style just always be null for a table? If the first case, I'm thinking trying to use config.Style first and moving to config.Parent?.Style if the node doesn't have a style specified would be the way to go.

Copy link
Contributor Author

@mark-sil mark-sil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aror92)


Src/xWorks/WordStylesGenerator.cs line 974 at r1 (raw file):

Previously, aror92 (Ariel Ror.) wrote…

One more question: do table nodes ever have a style specified, or will config.Style just always be null for a table? If the first case, I'm thinking trying to use config.Style first and moving to config.Parent?.Style if the node doesn't have a style specified would be the way to go.

The table could have a style, which should apply to the content of the table. This code is intended to make the entire table line up with the indenting used for the entry, so it should always use the parent style for indenting/justification.

Copy link
Contributor

@aror92 aror92 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mark-sil)

@mark-sil mark-sil merged commit c351ca8 into release/9.1 Apr 30, 2024
5 checks passed
@mark-sil mark-sil deleted the LT-21761a branch April 30, 2024 19:49
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