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

Update layout #8510

Merged
merged 1 commit into from Oct 20, 2020
Merged

Update layout #8510

merged 1 commit into from Oct 20, 2020

Conversation

RussKie
Copy link
Member

@RussKie RussKie commented Oct 3, 2020

Relates to #6183

Resolves #8521

Proposed changes

  • Reduce margins and paddings by at least 12px in each dimension
    • Update FormClone
    • Update FormInit
    • Update FormStatus
  • Fix Accept and Cancel buttons that get unset by adding buttons to the controls panel.

Screenshots

Before After
image image
image image
image image
.. image
.. image

✒️ I contribute this code under The Developer Certificate of Origin.

@ghost ghost assigned RussKie Oct 3, 2020
@codecov
Copy link

codecov bot commented Oct 3, 2020

Codecov Report

Merging #8510 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8510      +/-   ##
==========================================
- Coverage   55.05%   55.05%   -0.01%     
==========================================
  Files         901      901              
  Lines       65043    65047       +4     
  Branches    11726    11726              
==========================================
+ Hits        35811    35813       +2     
+ Misses      26475    26473       -2     
- Partials     2757     2761       +4     
Flag Coverage Δ
#production 41.99% <100.00%> (+<0.01%) ⬆️
#tests 94.81% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

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

Looks better - I want smaller margins (discussed in the issue) but I will not make it better myself

GitUI/GitExtensionsDialog.Designer.cs Outdated Show resolved Hide resolved
GitUI/CommandsDialogs/FormInit.Designer.cs Outdated Show resolved Hide resolved
@mstv
Copy link
Member

mstv commented Oct 3, 2020

I would right-align the bottom buttons with the boxes in the client area.

@ghost
Copy link

ghost commented Oct 4, 2020

image

@RussKie
Copy link
Member Author

RussKie commented Oct 4, 2020 via email

@ghost
Copy link

ghost commented Oct 4, 2020

Yes, 18 is the new reduced padding. The middle between 12 and 24.

Red color shows indents that do not correspond to the declared 18px.
Other colors indicate areas for possible improvement.

@ghost
Copy link

ghost commented Oct 4, 2020

Change paddings for base ControlsPanel.
ivangrek@1989c24

image

@ghost
Copy link

ghost commented Oct 4, 2020

For such indents, the button looks better than 32px. For comparison.

image

@RussKie
Copy link
Member Author

RussKie commented Oct 4, 2020

Change paddings for base ControlsPanel.
ivangrek@1989c24

image

Thank you, but you have probably missed my reply in #6183 (comment).
The spacing in the controls panel is (almost) aligned with the Task Dialog. Increasing the vertical paddings to 18px makes it look odd, and won't go well with either Gerhard or Michael.

@ghost
Copy link

ghost commented Oct 4, 2020

I would then recommend using harmonic values ​​for derived margins: 1:1, 1:2, 1:3, 1:4.
This is also an argument for 12px.

@RussKie
Copy link
Member Author

RussKie commented Oct 4, 2020

Windows itself is inconsistent (haha!), but it generally doesn't have the same value for v- and h-padding for buttons - the TD is a perfect example of this.

There's another aspect to this - buttons placed at the bottom panel invoke actions that apply to the whole dialog, whilst there may be other action elements that don't. To make this fact more obvious, and for a consistent UX this panel is visually separated from the main content. However the buttons aren't meant to dominate visually and steal user's focus from the main content.
There's a whole science around UX, useability and "comfortable" spacing (which I'm not sufficiently equipped to have deep discussions about), but I based on my experience and observations giving buttons as much v-padding makes the bottom more prominent that necessary, and thus divert some of the user's attention from the task at hand.

On the other hand, if the main content h-paddings are too small - it makes a dialog visually unpleasant. And hence the recommendations for 12/24 paddings.
Since a lot of our form are resizable, and generally larger than 640px, making h-paddings 12px doesn't feel like a good UX. Dynamically re-calc paddings feels even worse decision. And since 24px is regarded by few members of the team as "too wide", 18 feels like a good middle ground.

@ghost
Copy link

ghost commented Oct 4, 2020

I thought these tasks were aimed at developing a common approach. And so ok.

@gerhardol
Copy link
Member

On the other hand, if the main content h-paddings are too small - it makes a dialog visually unpleasant. And hence the recommendations for 12/24 paddings.
Since a lot of our form are resizable, and generally larger than 640px, making h-paddings 12px doesn't feel like a good UX.

This is where we disagre - I do not find 12 unpleasant for large windows

Dynamically re-calc paddings feels even worse decision.

yes...

And since 24px is regarded by few members of the team as "too wide", 18 feels like a good middle ground.

But the recommendation is to have multiple by 4, 18 may scale worse

@RussKie
Copy link
Member Author

RussKie commented Oct 19, 2020

@msftbot merge in 24 hours

@ghost ghost added the status: auto merge label Oct 19, 2020
@ghost
Copy link

ghost commented Oct 19, 2020

Hello @RussKie!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Tue, 20 Oct 2020 12:53:42 GMT, which is in 1 day

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

Reduce margins and paddings
* Update `FormClone`
* Update `FormInit`
* Update `FormStatus`
* Update `FormCheckoutBranch`

Relates to gitextensions#6183

Fix Accept and Cancel buttons that get unset by adding buttons to the controls panel.

Resolves gitextensions#8521
@gerhardol

This comment has been minimized.

@RussKie
Copy link
Member Author

RussKie commented Oct 19, 2020 via email

@RussKie RussKie merged commit 0b5ecd2 into gitextensions:master Oct 20, 2020
@RussKie RussKie deleted the Update_layout branch October 20, 2020 06:09
@ghost ghost added this to the 4.0 milestone Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checkout windows in GitExtensions 4.0 CI build doesn't repond to 'Enter' key
3 participants