search

shpaass / yafc-ce

Powerful Factorio calculator/analyser that works with mods
GNU General Public License v3.0
40 stars 15 forks source link

Use calculated width for first column of Summary page #167

Closed veger closed 4 weeks ago

veger commented 4 weeks ago

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 commented 4 weeks ago

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...

shpaass commented 4 weeks ago

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 commented 4 weeks ago

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?

shpaass commented 4 weeks ago

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 commented 4 weeks ago

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

shpaass commented 4 weeks ago

Reading it now