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-21672: Add styles to table cells #17

Merged
merged 5 commits into from
Apr 4, 2024
Merged

Conversation

mark-sil
Copy link
Contributor

@mark-sil mark-sil commented Mar 27, 2024

This adds support for when a style is applied to a specific cell or when different styles are applied to different pieces of the text in a cell.

Note: The changes in SetRunStyle() also add support for styles that are applied to all or a portion of the text in other fields (notes, custom fields …).

Change-Id: Id1ca7ccfaeaad7e7ea7e781263151b1714d9d90f


This change is Reviewable

This adds support for when a style is applied to a specific
cell or when different styles are applied to different pieces
of the text in a cell.

Note: The changes in SetRunStyle() also add support for styles
that are applied to all or a portion of the text in other
fields (notes, custom fields …).

Change-Id: Id1ca7ccfaeaad7e7ea7e781263151b1714d9d90f
@mark-sil mark-sil requested a review from aror92 March 27, 2024 16:55
Copy link
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 5 files at r1, all commit messages.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @aror92 and @mark-sil)


Src/xWorks/ConfiguredLcmGenerator.cs line 2811 at r1 (raw file):

				var css = cssStyle.ToString();

				settings.ContentGenerator.SetRunStyle(writer, config, writingSystem, css, style);

I think a different refactoring might be better here. What if instead of directly calling the CssGenerator here we use the settings.StyleGenerator and pass that in. css should be renamed style in the interface, and the writing system is still necessary for word, but it would unify the design here.

Code quote:

				var cssStyle = CssGenerator.GenerateCssStyleFromLcmStyleSheet(style,
					settings.Cache.WritingSystemFactory.GetWsFromStr(writingSystem), settings.PropertyTable);
				var css = cssStyle.ToString();

				settings.ContentGenerator.SetRunStyle(writer, config, writingSystem, css, style);

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: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @aror92 and @jasonleenaylor)


Src/xWorks/ConfiguredLcmGenerator.cs line 2811 at r1 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

I think a different refactoring might be better here. What if instead of directly calling the CssGenerator here we use the settings.StyleGenerator and pass that in. css should be renamed style in the interface, and the writing system is still necessary for word, but it would unify the design here.

Hi Jason, I don't understand what you are recommending. How should settings.StylesGenerator be used? I'm not sure if this is relevant, but the 'config' doesn't contain the style that we want.

@jasonleenaylor
Copy link
Contributor

Src/xWorks/ConfiguredLcmGenerator.cs line 2811 at r1 (raw file):

Previously, mark-sil (Mark Kidder) wrote…

Hi Jason, I don't understand what you are recommending. How should settings.StylesGenerator be used? I'm not sure if this is relevant, but the 'config' doesn't contain the style that we want.

After our chat in slack the new plan is to make sure that we are handling the export specific code in the right class. So in this case moving the css generating into the xhtml export is the change. Getting the ILcmStylesGenerator to handle this wasn't the best route.

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: 3 of 5 files reviewed, all discussions resolved (waiting on @aror92)


Src/xWorks/ConfiguredLcmGenerator.cs line 2811 at r1 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

After our chat in slack the new plan is to make sure that we are handling the export specific code in the right class. So in this case moving the css generating into the xhtml export is the change. Getting the ILcmStylesGenerator to handle this wasn't the best route.

I’m not sure how to proceed with this.  Should I add an ‘error’ parameter to SetRunStyle() and keying off of that in the xhtml generator to create the css for large red text?

mark-sil and others added 3 commits April 2, 2024 13:41
This adds support for when a style is applied to a specific
cell or when different styles are applied to different pieces
of the text in a cell.

Note: The changes in SetRunStyle() also add support for styles
that are applied to all or a portion of the text in other
fields (notes, custom fields …).

Change-Id: Iee3a869f049f876e4b2cb03c6cbb5896b6a95826
Copy link
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @aror92)

@mark-sil mark-sil merged commit 939870c into release/9.1 Apr 4, 2024
5 checks passed
@mark-sil mark-sil deleted the Features/LT-21672a branch April 4, 2024 18:55
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