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

Change TextImageRelation For "Reset All" Button #8761

Merged
merged 6 commits into from Feb 15, 2021
Merged

Change TextImageRelation For "Reset All" Button #8761

merged 6 commits into from Feb 15, 2021

Conversation

thimmy687
Copy link
Contributor

@thimmy687 thimmy687 commented Jan 17, 2021

Change TextImageRelation

For some languages e.g. german the button is to small and
the icon was cutted and moved a little bit down.
For this the TextImageRelation was changed to overlay to get
the same behavior as the other buttons.

Fixes #6548 (partially)

Proposed changes

  • change the TextImageRelation to avoid the cutting of the icon in front of the button

Screenshots

Before

image

After

image

Test methodology

  • Manual test

Test environment(s)

  • Windows 10

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

@ghost ghost assigned thimmy687 Jan 17, 2021
@codecov
Copy link

codecov bot commented Jan 17, 2021

Codecov Report

Merging #8761 (c00afb0) into master (53e88d1) will increase coverage by 0.49%.
The diff coverage is 48.64%.

@@            Coverage Diff             @@
##           master    #8761      +/-   ##
==========================================
+ Coverage   55.66%   56.16%   +0.49%     
==========================================
  Files         894      919      +25     
  Lines       64855    65497     +642     
  Branches    11870    11992     +122     
==========================================
+ Hits        36103    36784     +681     
+ Misses      25809    25716      -93     
- Partials     2943     2997      +54     
Flag Coverage Δ
production 43.32% <40.30%> (+0.19%) ⬆️
tests 94.47% <81.75%> (+0.15%) ⬆️

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

@mstv mstv added 📜 status: needs cla The author needs to update contributors.txt before reviewing or merging can proceed area: translation 🖊️ status: cla signed and removed 📜 status: needs cla The author needs to update contributors.txt before reviewing or merging can proceed labels Jan 17, 2021
Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

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

Could you manually apply the essential changes to the designer file, please?
It seems that the modifyCommitMessageButton or its Text got lost somehow.
Feel free to force-push to this branch.

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

The diff contains too many unrelated changes that makes it impossible to reason about the proposed change.

Please remove all irrelevant changes and keep the actual change.

@ghost ghost added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jan 18, 2021
for longer translated languages more button space is required.
@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jan 22, 2021
@thimmy687
Copy link
Contributor Author

the designer is doing so much things in the background :/

But I removed the spam now

@pmiossec
Copy link
Member

the designer is doing so much things in the background :/

Even when running VisualStudio in no scale mode? https://github.com/gitextensions/gitextensions/wiki/Guidelines:-UI-design

Side note for dev team: I don't know how to make contributors aware that it is really important to update forms in no scale mode before they start to update the forms....

@pmiossec
Copy link
Member

But I removed the spam now

Sorry to have to tell you but there are still a lot of whitespaces added after // comments that need to be removed 😞

@RussKie
Copy link
Member

RussKie commented Jan 23, 2021

I've run it locally, and I think there is a little more to be done, the text still looks squashed:
image

The layouts of the panel and buttons need to be changed to be fluent and autosize based on content.

@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jan 23, 2021
@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jan 23, 2021
@thimmy687
Copy link
Contributor Author

I've run it locally, and I think there is a little more to be done, the text still looks squashed:
image

The layouts of the panel and buttons need to be changed to be fluent and autosize based on content.

I think in the released version other text strings are in place, for this is it fitting

image

@RussKie
Copy link
Member

RussKie commented Jan 23, 2021

I think in the released version other text strings are in place, for this is it fitting

image

No, it isn't. You can see the icon on the 4th button is lowered, meaning the text is wrapped.

@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jan 23, 2021
@thimmy687
Copy link
Contributor Author

thimmy687 commented Jan 23, 2021

I think in the released version other text strings are in place, for this is it fitting
image

No, it isn't. You can see the icon on the 4th button is lowered, meaning the text is wrapped.

The wrapping is no longer the case, because I changed the TextImageRelation to default (overlay)

can you tell me what will be the German text in the next release?

If it is "Ungestagte Änderungen" everything is fine, if it is " "Ungestagte Änderungen zurücksetzen" we will need much more space for this button, in this case I would add a additional change.

This is how it looks in debug mode now:

image

But I guess in the 3rd button the word "zurücksetzen" will be removed, like in version 3.4.3 and than it looks good

@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jan 23, 2021
@gerhardol
Copy link
Member

But I guess in the 3rd button the word "zurücksetzen" will be removed, like in version 3.4.3 and than it looks good

The current translation "Reset Unstaged Changes" is "Alle ungestagten Änderungen zurücknehmen" (and for "Reset All Changes" "Alle Änderungen zurücknehmen". You can find this in the .xlf files.
This is updated by translators on Transifex.
The translation soures for next version (3.5?) is not pushed yet to Transifex, usually done some weeks before the release.

@RussKie
Copy link
Member

RussKie commented Jan 24, 2021

The translation soures for next version (3.5?) is not pushed yet to Transifex, usually done some weeks before the release.

The translations are pushed to transifex constantly, it just we don't pull updated translations that often.

The current translation "Reset Unstaged Changes" is "Alle ungestagten Änderungen zurücknehmen" (and for "Reset All Changes" "Alle Änderungen zurücknehmen".

Wow, that's way too long to comfortably present on the button without making the button take half the screen. 🤔

@RussKie
Copy link
Member

RussKie commented Jan 24, 2021

#8791 brings the latest translations

@RussKie
Copy link
Member

RussKie commented Jan 24, 2021

But I guess in the 3rd button the word "zurücksetzen" will be removed, like in version 3.4.3 and than it looks good

@mstv what do you think?

@thimmy687 we don't generally control translations, those are done by the community at Transifex. You are welcome to fix it, though there's no guarantee someone won't attempt to change it later.

@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jan 24, 2021
@mstv
Copy link
Member

mstv commented Jan 24, 2021

But I guess in the 3rd button the word "zurücksetzen" will be removed, like in version 3.4.3 and than it looks good

It is a valid option to remove the verb, although this will only be understandable in combination with the icon.
I agree that "zurücknehmen" would have been the better translation.
Perhaps we should add a tooltip for translations which must be truncated.

But I strongly prefer all development tools in English.
Amongst others, Microsoft has often chosen the wrong translation, which then must be translated back in order to understand the real meaning. There are several permanent irritants.
And there is no real translation for e.g. "unstaged", just a German declension of the English word: ungestagt.
Of course, just my 2 ct.

@thimmy687
Copy link
Contributor Author

but the TextImageRelation change is still valid ... this avoid the cutting/wrapping of the icon in the button

So for me we could merge this still

@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jan 30, 2021
@RussKie
Copy link
Member

RussKie commented Feb 10, 2021

but the TextImageRelation change is still valid ... this avoid the cutting/wrapping of the icon in the button

So for me we could merge this still

No, the change is not correct because the last button isn't showing the full text:

<trans-unit id="ResetUnStaged.Text">
<source>Reset u&amp;nstaged changes</source>
<target>U&amp;ngestagte Änderungen zurücksetzen</target>

This is how it supposed to look in German language:
image

But the buttons must not be as wide for English, or any other language. The layouts of the panel and buttons need to be changed to be fluent and autosize based on content.

@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Feb 10, 2021
@pmiossec
Copy link
Member

But the buttons must not be as wide for English, or any other language. The layouts of the panel and buttons need to be changed to be fluent and autosize based on content.

I'm far to be an expert in winforms, so I'm really interested to know how to do that (without code behind) because that have always been very difficult to manage autosizing of button containing an image. I always thought that the handling of this case should have been better handled by winforms....

@thimmy687
Copy link
Contributor Author

thimmy687 commented Feb 10, 2021

we can also keep the origin size and just change the TextImageRelation

It's not the best solution for translations but the easiest at the moment.

Better integrations are available with other frameworks like WPF

@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Feb 10, 2021
@RussKie
Copy link
Member

RussKie commented Feb 10, 2021 via email

@thimmy687 thimmy687 changed the title Change Button Size and TextImageRelation Change TextImageRelation For Reset All Button Feb 12, 2021
@RussKie RussKie changed the title Change TextImageRelation For Reset All Button Change TextImageRelation For "Reset All" Button Feb 15, 2021
@RussKie RussKie merged commit ac88bd2 into gitextensions:master Feb 15, 2021
@ghost ghost added this to the 3.6 milestone Feb 15, 2021
@RussKie
Copy link
Member

RussKie commented Feb 15, 2021

Thank you

@RussKie RussKie modified the milestones: 3.6, 3.5 Feb 15, 2021
RussKie pushed a commit that referenced this pull request Feb 15, 2021
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.

Some texts in german are too big for some buttons
5 participants