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

Improve fax menu #33653

Closed
wants to merge 6 commits into from
Closed

Conversation

SpyDev14
Copy link
Contributor

@SpyDev14 SpyDev14 commented Nov 29, 2024

Subdivision of: #33632 pr

About the PR

Improved the fax menu. Now it doesn't look so scary in Russian localization.

Why / Balance

It was the essence of pure terror

Media

Before

2024-11-28_23-45-44
2024-11-28_23-49-03

on RU locale

image

After

2024-12-01_18-12-43
2024-12-01_18-12-04

Requirements

Changelog

🆑

  • tweak: Fax UI Menu little reworked
  • fix: Fix fax UI menu resizable

@github-actions github-actions bot added size/L Denotes a PR that changes 1000-4999 lines. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. Changes: UI Changes: Might require knowledge of UI design or code. Changes: No C# Changes: Requires no C# knowledge to review or fix this item. labels Nov 29, 2024
@beck-thompson beck-thompson added P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. D2: Medium Difficulty: A good amount of codebase knowledge required. S: Needs Review Status: Requires additional reviews before being fully accepted T: UI / UX Improvement Type: UI and player facing interactive graphical interfaces A: General Interactions Area: General in-game interactions that don't relate to another area. and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Nov 29, 2024
@slarticodefast
Copy link
Member

Small request: Could you reorganize the buttons a little so they are better grouped by their purpose?

  • Move "Print File" to "Copy" and "Send"
  • Move "Refresh" to the receiver of the fax
  • Standardize their size

Small Photoshop mockup:
391141400-551d0ed7-3bea-469b-98f8-75c02991326e

@slarticodefast slarticodefast added S: Awaiting Changes Status: Changes are required before another review can happen and removed S: Needs Review Status: Requires additional reviews before being fully accepted labels Nov 29, 2024
@IProduceWidgets
Copy link
Contributor

Does this effect the admin fax ui?

@SpyDev14

This comment was marked as abuse.

@SpyDev14

This comment was marked as abuse.

@slarticodefast
Copy link
Member

I looked at it again and this does not seem to implement the requested changes yet.
Currently it looks like this:
grafik
But a better layout would be this in my opinion:
391162573-c238c24c-619c-455b-b391-2498f9dc630c
Could you make it look like that?

@SpyDev14

This comment was marked as abuse.

@slarticodefast
Copy link
Member

Of course they are not implemented, they look bad. They are illogical and look crooked.

What about them is illogical in your opinion?
My reasoning for moving them is:

  • The "Refresh" button should be next to the list of fax receivers because it refreshes that list. Otherwise you would not know what it will refresh.
  • The "Print File" button is next to "Copy" and "Send" because those are the three functions the fax machine has. The "Print File" button has nothing to do with the "Refresh" button, so they should not be grouped together.

@SpyDev14

This comment was marked as abuse.

@eoineoineoin
Copy link
Member

Slarticodefast is correct here. The UI elements which have related functions should be beside each other. You know what every button does because you made the UI, but consider a player who might be looking at this UI for the first time - for example that player will see a "Refresh" button which is somehow connected to "Print File," so they will wonder what relationship those functions have with each other, when in reality, those two functions are unconnected to each other.. In Slarticodefast's mockup, the "Refresh" button is beside the list of destinations because it refreshes that list. I think it could be improved even further, but it's a great suggestion. Here's a short article about this design concept with links you can use to learn more: https://lawsofux.com/law-of-proximity/

On a non-tehcnical level, your comments feel a little aggressive. Please bear in mind that everyone on this repository is a volunteer and is working in their free time to make the game better. I appreciate that English may not be your first language, but please consider toning down the language - if you're going to insult volunteers who are trying to help, I think you'll find that people will not want to work with you, no matter how good your contributions are.

Finally, your original PR description contains "Now it doesn't look so scary in Russian localization" - it would be good to know what the problem is so that we could learn to avoid making mistakes which cause problems for translations in the future.

@SpyDev14

This comment was marked as abuse.

@SpyDev14

This comment was marked as abuse.

@SpyDev14

This comment was marked as abuse.

@SpyDev14

This comment was marked as abuse.

@slarticodefast
Copy link
Member

slarticodefast commented Dec 2, 2024

Personally I still think the Refresh button should be best placed next to the receiver. But we can wait for someone else to chime in for that.

Thanks to eoin for putting what I was trying to explain into technical terms.

@SpyDev14

This comment was marked as abuse.

@SpyDev14

This comment was marked as abuse.

@slarticodefast
Copy link
Member

Like Ed suggested the Refresh button might not be needed at all if you just refresh it when opening the UI instead. That would be a good solution in my opinion.

@eoineoineoin
Copy link
Member

Yes, ideally, the refresh button wouldn't be necessary at all, would certainly be an improvement if it could be removed. However, the paper type and print file button are still separated. I think this is closer to what a user would find more intuitive, but maybe needs headers to distinguish the "fax" controls from the "print" controls:

2024-12-02_17-35

And of course, if we wanted to avoid bikeshedding over this, we could just fix the issue when the window is resized - that's a one-line diff.

@SpyDev14

This comment was marked as abuse.

@eoineoineoin
Copy link
Member

I made a quick edit to your branch to demonstrate how the UI could be laid out grouping the controls by function.

@SpyDev14

This comment was marked as abuse.

@Everturning
Copy link

why are all the PR author's comments marked as abuse tf?

@slarticodefast
Copy link
Member

why are all the PR author's comments marked as abuse tf?

They were blocked from the org for behavior in various PRs, even after already being warned by a moderator.
I'm not sure what this means for their PRs because those have been good changes. In this case the suggestion for the UI @eoineoineoin made is a good solution in my opinion. Eoin, could you maybe make a PR for that? While crediting the changes VlaDOS made of course.

@eoineoineoin
Copy link
Member

That's unfortunate; their behaviour may not have been very good, but it does seem they were genuinely trying to make some improvements. I'd be happy to put the work in to polish and finish their PRs, since they fix issues in non-English builds.

In terms of this one, what would your preference be for how to get it in? Make a new PR, or push to this branch (not sure I have the permissions to do this, so you might have to do the push). I've pushed that change to eoineoineoin@8899af2 I still think it could be improved, maybe some headers on each section or something, but it's definitely an improvement on the current UI.

@slarticodefast
Copy link
Member

Easiest would be to make a new PR, but make sure to keep the previous commits so that VlaDOS gets contributed for their work.

@eoineoineoin eoineoineoin mentioned this pull request Dec 11, 2024
2 tasks
@TheShuEd
Copy link
Member

Due to the author being banned from the repository, we will be closing this PR. Other contributors may continue the work by opening up a new PR. Should the author successfully appeal the ban, the PR may be re-opened.

@TheShuEd TheShuEd closed this Dec 20, 2024
@SpyDev14 SpyDev14 deleted the Improve-Fax-menu branch January 7, 2025 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: General Interactions Area: General in-game interactions that don't relate to another area. Changes: No C# Changes: Requires no C# knowledge to review or fix this item. Changes: UI Changes: Might require knowledge of UI design or code. D2: Medium Difficulty: A good amount of codebase knowledge required. P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. S: Awaiting Changes Status: Changes are required before another review can happen size/L Denotes a PR that changes 1000-4999 lines. T: UI / UX Improvement Type: UI and player facing interactive graphical interfaces
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants