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

feat: add text justification style #2410

Merged

Conversation

Vahdanian
Copy link
Contributor

Hi, thanks for reviewing my pull request. This pull request intends to fix [https://github.com//issues/1995](this issue)

Here are some key points worth mentioning:
1- After initial investigation, I found that Justify property in "SummaryTableColumn" class does not have impact on aligning the contents of summary table. It only effects the header separator of table. (See the first attachment please)

2- I managed to add the "DefaultTextJustification" property to "SummaryStyle" and use the style to set "Justify" property inside "SummaryTableColumn". I changed the "BuildStandardText" and "BuildBoldText" methods to use "Justify" property.

3- Since I changed how the Justify Property was being initialized and the previous value was only used inside "GetJustificationIndicator" method, I changed the method to read from "IsNumeric" value of passed column. Exactly like the previous value of "Justify" property

4- I modified SummaryTableTests class and added a new method to MockFactory class to test the new style. CreateSummary method inside MockFactory did not used the specified style of config object and directly called the constructor of Summary class where style was initialized with default instance. So I added a new parameter for style.

5- Finally I modified methods "SummaryStyle" class and added a new method in "HashCode" struct to support the newly added style "DefaultTextJustification".

6- You can see the final result in second attachment.

Justify not working on columns
after changes

@timcassell timcassell linked an issue Aug 18, 2023 that may be closed by this pull request
@timcassell
Copy link
Collaborator

It looks strange to me that there is no space on the left when it's left-justified, but there is a space on the right when it's right-justified.

@Vahdanian
Copy link
Contributor Author

@timcassell
Sorry to bother but I needed to ask. What do you suggest for the tests that use a predefined .txt files in "VerifiedFiles" directory to verify the output? I started to notice some of these tests are failing because of new justify style (You can find one here). Shall I update the txt files to expect columns to be justified to left by default? Or you have something else in mind? please let me know.

@timcassell
Copy link
Collaborator

Shall I update the txt files to expect columns to be justified to left by default?

That sound reasonable to me. Especially since the old test expects it (but seems broken currently).

@Vahdanian
Copy link
Contributor Author

@timcassell
I separated styling for numeric and text columns and fixed broken tests, Can you review the changes?

@Vahdanian
Copy link
Contributor Author

It looks strange to me that there is no space on the left when it's left-justified, but there is a space on the right when it's right-justified.

This is fixed in the second commit

@Vahdanian
Copy link
Contributor Author

@timcassell
Hi, extra spaces are removed in 3d commit and numeric columns are now justified to right by default

@Vahdanian
Copy link
Contributor Author

@timcassell
Do you have a clue why some of the tests failed? It says some of the memory diagnoser tests are failing but I checked them on my machine and they were fine.

@timcassell
Copy link
Collaborator

timcassell commented Aug 29, 2023

@timcassell Do you have a clue why some of the tests failed? It says some of the memory diagnoser tests are failing but I checked them on my machine and they were fine.

It's unrelated. Those tests are flaky.

Copy link
Collaborator

@timcassell timcassell left a comment

Choose a reason for hiding this comment

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

Thanks @Vahdanian!

@timcassell timcassell merged commit b035d90 into dotnet:master Sep 1, 2023
7 checks passed
@timcassell timcassell added this to the v0.13.8 milestone Sep 1, 2023
@Vahdanian
Copy link
Contributor Author

Thanks @Vahdanian!

You're very welcome! I'm glad I could contribute to the project 😄

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.

Left Justification option for summary table columns
2 participants