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

Use calculated width for first column of Summary page #167

Merged
merged 2 commits into from
Jun 21, 2024

Conversation

veger
Copy link
Collaborator

@veger veger commented Jun 20, 2024

With this PR the fixed width (introduced by #38) will be no more and the column width is dynamically calculated depending on the names of the production sheets.


This change is part of a larger set of changes that I am planning to commit, but those changes need some additional love... (It works, but I broke some things that should not be broken).

This change is ready and works separately and adds value to YAFC already.

I had to apply one small hack to make this a stand-alone PR, which is making firstColumnWidth static. In the upcoming PR it will be a regular field of its class.

@veger veger requested review from DaleStan and shpaass June 20, 2024 23:04
DaleStan
DaleStan previously approved these changes Jun 21, 2024
Copy link
Collaborator

@DaleStan DaleStan left a comment

Choose a reason for hiding this comment

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

Looks good to me

@veger
Copy link
Collaborator Author

veger commented Jun 21, 2024

I'll rebase against master (without the merge commit) and when I have another approval I'll merge

@have-fun-was-taken maybe we should disable the reviews getting dismissed on new commits? This requires re-reviewing over and over...

@veger veger force-pushed the summary-calculate-first-column-width branch 2 times, most recently from e05e9ea to 51ea298 Compare June 21, 2024 07:11
@shpaass
Copy link
Owner

shpaass commented Jun 21, 2024

maybe we should disable the reviews getting dismissed on new commits?

The idea was that it improves the code review by forcing the reviewer to check the final code. The reality is that it became annoying when each rebase must be re-approved. Sure, I'll disable it.

@veger
Copy link
Collaborator Author

veger commented Jun 21, 2024

I understand the initial idea, and I support this completely in order to be sure the changes are noticed and reviewed again.

Maybe we can have a 'guideline' to manually ask for an re-approval when we updated the PR by addressing the feedback?

veger added 2 commits June 21, 2024 09:19
So it is much more convenient to find the width of a text without
actually allocating the rectangle in the GUI.
This prevent unnecessary/reserved width for this colum
@veger veger force-pushed the summary-calculate-first-column-width branch from 51ea298 to 0ac3048 Compare June 21, 2024 07:19
@shpaass
Copy link
Owner

shpaass commented Jun 21, 2024

Maybe we can have a 'guideline' to manually ask for an re-approval when we updated the PR by addressing the feedback?

If we have too many rules, it might feel stifling.
Rather, I think a precedent-based approach is fitting there. In other words, if someone merges bad things under the guise of previous approvals, then we'll have a talk and add a rule. I doubt it's a default behavior of anyone, so no rule required at the moment.

@veger
Copy link
Collaborator Author

veger commented Jun 21, 2024

@have-fun-was-taken can you re-approve this PR, so we can merge it?

@shpaass
Copy link
Owner

shpaass commented Jun 21, 2024

Reading it now

@shpaass shpaass merged commit 450d3a4 into shpaass:master Jun 21, 2024
1 check passed
@veger veger deleted the summary-calculate-first-column-width branch June 21, 2024 07:48
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