Skip to content

Conversation

@phanithinks
Copy link
Contributor

@phanithinks phanithinks commented Jan 11, 2023

  • PR Description
    Fix for not able to set the vertical screen mode as default  #2319
    new gui.screenMode key added to configuration

  • Please check if the PR fulfills these requirements

    • Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.go generate)
    • Code has been formatted (see here)
    • Tests have been added/updated (see here for the integration test guide)
    • Text is internationalised (see here)
    • Docs (specifically docs/Config.md) have been updated if necessary
    • You've read through your own file changes for silly mistakes etc

@phanithinks phanithinks changed the title Fixes #2319 Added new gui.screenMode key to config Jan 11, 2023
@jesseduffield
Copy link
Owner

Looks good @phanithinks . One thing: now that the user is seeing the name 'screenMode', it's worth asking: is this an intuitive name? I think 'windowSize' would be better

@phanithinks
Copy link
Contributor Author

phanithinks commented Jan 16, 2023

Looks good @phanithinks . One thing: now that the user is seeing the name 'screenMode', it's worth asking: is this an intuitive name? I think 'windowSize' would be better

Thank you @jesseduffield for reviewing. I changed as per your suggestion in configuration.

@phanithinks
Copy link
Contributor Author

@jesseduffield Do I need to make any more changes to this pr ? Please let me know

Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

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

Sorry for the late re-review, I've left one more comment

@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2023

Uffizzi Preview Environment deployment-13017

☁️ https://app.uffizzi.com/github.com/jesseduffield/lazygit/pull/2358

📄 View Application Logs etc.

What is Uffizzi? Learn more

@jesseduffield jesseduffield merged commit c0e8057 into jesseduffield:master Feb 1, 2023
@jesseduffield
Copy link
Owner

Merged. Thanks for making this @phanithinks !

@mikeduminy
Copy link

Thank you! Just what I was looking for :D

One thing to note, the commands for switching between these visual modes refer to this feature as screenMode, not windowSize. To me windowSize feels like it's describing the size of the overall gui window and not the layout. I'd rather go with gui.layout or gui.defaultMainPanelSize.

@jesseduffield
Copy link
Owner

hmm if there's a time to change that it would be now. I actually think gui.focusedPanelSize makes the most sense, as that's the panel that would be resized. It is a shame we just released this haha, meaning we'd have to be backwards compatible

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