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.

Sign up for GitHub

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

Jump to bottom

Fix army split dialog text overflowing the dialog and add troops name #7956

Merged
merged 29 commits into from
Mar 6, 2024
Merged

Fix army split dialog text overflowing the dialog and add troops name #7956

merged 29 commits into from
Mar 6, 2024

Conversation

lioror
Copy link
Contributor

@lioror lioror commented Oct 17, 2023 •
edited
Loading

Fixing this issue
Adding the units name in the army split dialog.
Adjust dialog height for multi line text, ex. translations.

Here is the new look:
image

2 lines:
image

github-actions[bot]
github-actions bot reviewed Oct 18, 2023
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

src/fheroes2/dialog/dialog_selectcount.cpp Outdated Show resolved Hide resolved
src/fheroes2/dialog/dialog_selectcount.cpp Outdated Show resolved Hide resolved
src/fheroes2/dialog/dialog_selectcount.cpp Outdated Show resolved Hide resolved
src/fheroes2/dialog/dialog_selectcount.cpp Outdated Show resolved Hide resolved
src/fheroes2/dialog/dialog_selectcount.cpp Outdated Show resolved Hide resolved
src/fheroes2/dialog/dialog_selectcount.cpp Outdated Show resolved Hide resolved
@Districh-ru Districh-ru added bug Something doesn't work ui UI/GUI related stuff labels Oct 18, 2023
@Districh-ru Districh-ru added this to the 1.0.10 milestone Oct 18, 2023
@oleg-derevenetz oleg-derevenetz linked an issue Oct 18, 2023 that may be closed by this pull request
Army/artifact trading dialog: text artifact on the picture of the right hero (german translation). #7551
Closed
2 tasks
@lioror
Copy link
Contributor Author

lioror commented Oct 22, 2023

can someone please assist?

@zenseii
Copy link
Collaborator

zenseii commented Oct 22, 2023

can someone please assist?

Hi, @lioror just check the details of the check that failed and it will show you what needs to be removed with a - and what should be added with a +. Comparing these will help you figure out what is the problem with the code style.

@ihhub
Copy link
Owner

ihhub commented Nov 15, 2023

Hi @lioror , please fix code style issues.

@lioror
Copy link
Contributor Author

lioror commented Nov 16, 2023

@ihhub done

ihhub
ihhub requested changes Nov 16, 2023
View reviewed changes
Copy link
Owner

@ihhub ihhub left a comment

Choose a reason for hiding this comment

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

Hi @lioror, I left multiple comments in this pull request. Could you please take a look at them and also attach screenshots of this dialog after your changes in the first message of this PR aka PR's description?

Moreover, please correct the title of your pull request to reflect the changes you've done. Right now it is too generic and tells nothing what this PR is about.

src/fheroes2/dialog/dialog_selectcount.cpp Outdated Show resolved Hide resolved
src/fheroes2/dialog/dialog_selectcount.cpp Outdated Show resolved Hide resolved
src/fheroes2/dialog/dialog_selectcount.cpp Outdated Show resolved Hide resolved
src/fheroes2/dialog/dialog_selectcount.cpp Outdated Show resolved Hide resolved
src/fheroes2/dialog/dialog_selectcount.cpp Outdated Show resolved Hide resolved
src/fheroes2/dialog/dialog_selectcount.cpp Outdated Show resolved Hide resolved
src/fheroes2/army/army_bar.cpp Show resolved Hide resolved
@ihhub ihhub modified the milestones: 1.0.10, 1.0.11 Nov 18, 2023
@ihhub ihhub requested review from ihhub and Branikolog December 1, 2023 15:59
@lioror lioror changed the title Army split dialog Fix army split dialog text overflowing the dialog and add troops name Dec 7, 2023
ihhub
ihhub requested changes Dec 13, 2023
View reviewed changes
Copy link
Owner

@ihhub ihhub left a comment

Choose a reason for hiding this comment

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

Hi @lioror , I put several comments here. Could you please take a look when you have time?

src/fheroes2/dialog/dialog_selectcount.cpp Outdated Show resolved Hide resolved
src/fheroes2/dialog/dialog_selectcount.cpp Outdated Show resolved Hide resolved
src/fheroes2/dialog/dialog_selectcount.cpp Outdated Show resolved Hide resolved
@ihhub ihhub modified the milestones: 1.0.11, 1.1.0 Dec 23, 2023
@lioror lioror marked this pull request as ready for review January 15, 2024 21:51
github-actions[bot]
github-actions bot reviewed Jan 15, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

src/fheroes2/dialog/dialog_selectcount.cpp Outdated Show resolved Hide resolved
@zenseii
Copy link
Collaborator

zenseii commented Feb 7, 2024

I suggest merging this after #8360 since having non uniform text alignment would be much better for this dialog.

@zenseii zenseii requested a review from ihhub February 13, 2024 14:40
@Branikolog
Copy link
Collaborator

Hi, @lioror , @zenseii and @ihhub .

English translation looks acceptable except the cases, when troops name is cut into two strings (I find it a little not pleasant visually, but still working for me).
image

But I'm afraid for other translations there could be issues with the noun form (like Russian has) which is not supported by the engine yet.
Maybe adding just a header with the creature name and leaving the dialog text as it is (without creature name) would be easier
to translate and more uniform?

@zenseii
Copy link
Collaborator

zenseii commented Feb 16, 2024

@Branikolog, I've made your requested change:
image

github-actions[bot]
github-actions bot reviewed Feb 16, 2024
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

src/fheroes2/dialog/dialog_selectcount.cpp Outdated Show resolved Hide resolved
Branikolog
Branikolog approved these changes Feb 16, 2024
View reviewed changes
Copy link
Collaborator

@Branikolog Branikolog left a comment

Choose a reason for hiding this comment

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

For me it looks and works well. 👍

@zenseii
Copy link
Collaborator

zenseii commented Mar 5, 2024

@ihhub, when you have time and feel like it, this PR is ready for review.

@ihhub ihhub merged commit 370f63a into ihhub:master Mar 6, 2024
20 checks passed
@ihhub
Copy link
Owner

ihhub commented Mar 6, 2024

@lioror , thank you so much for your contribution to the project!

@lioror lioror deleted the army-split-dialog branch April 19, 2024 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@zenseii zenseii zenseii left review comments

@Branikolog Branikolog Branikolog approved these changes

@ihhub ihhub ihhub approved these changes

Assignees

@lioror lioror

Labels
bug Something doesn't work ui UI/GUI related stuff
Projects
None yet
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.

Army/artifact trading dialog: text artifact on the picture of the right hero (german translation).
5 participants
@lioror @zenseii @ihhub @Branikolog @Districh-ru