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

Feature: Remove parentheses in price difference column. #9893

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

Just-Existing
Copy link
Contributor

@Just-Existing Just-Existing commented Mar 7, 2024

Feature

This PR addresses some of the problems described in issue #9203

Summary

I removed the parentheses from the price difference column.

Test Screenshots

Before:
price_difference_column_test_before
After:
price_difference_column_test_after
show parenthesis

Checklist

To fulfill the consensus on this pull request, these criteria must be met:

  • Remove parentheses.
  • Add option to put parens back in.

(I moved the original checklist to this gist)

@Hecter94 Hecter94 added the UI Everything related to the User Interface & Design (both artwork and code) label Mar 7, 2024
@OcelotWalrus
Copy link
Contributor

Why would you remove the parentheses?

@Zireael07
Copy link

@OcelotWalrus Read the linked issue, it explains why

@OcelotWalrus
Copy link
Contributor

Woops, sorry.

@mOctave
Copy link
Contributor

mOctave commented Mar 8, 2024

Honestly, I'm not a huge fan of removing the parentheses. I guess I should have mentioned this on the issue last year, but I seem to have missed it.

For me, the parentheses help to distinguish between what's a trade price and what isn't in a way that the +/- doesn't. It also helps pad one-digit differences so that they have a more similar width to everything else and don't stand out quite so much. With the image below, my eye is immediately drawn to metal, which is the most useless trade price on the entire board.
Screenshot 2024-03-07 at 9 42 19 PM

@Hecter94
Copy link
Member

Hecter94 commented Mar 8, 2024

The linked issue suggested this as a potential replacement for the parentheses if further visual separation was needed

I also recommend dropping the parentheses because they don't serve any function and take up precious space. The numbers are demarcated from the names by being right-aligned. If further visual separation is needed, I suggest drawing a vertical grid line between the 2 columns (rather than the unintentional squiggly line formed by the left parentheses).

Thoughts?

@Zireael07
Copy link

The grid is a good idea

@Amazinite
Copy link
Collaborator

Amazinite commented Mar 8, 2024

Keep in mind that #9721 is open as well for effectively the same issue.

@Zireael07
Copy link

In the words of a famous meme, why not both? They're orthogonal solutions AND map shades shouldn't be the only way to do it because of color-blindness

@pepoluan
Copy link

For me, the parentheses help to distinguish between what's a trade price and what isn't in a way that the +/- doesn't. It also helps pad one-digit differences so that they have a more similar width to everything else and don't stand out quite so much. With the image below, my eye is immediately drawn to metal, which is the most useless trade price on the entire board.

Not everyone has the same reaction.

For me, personally, removing the parentheses exposes the number better.

Maybe because I'm so familiar with Excel spreadsheets nearly every workday, but still we now have 2 anecdotal evidence that people have different preferences.

Maybe put in as an option? So those that prefer the parens can keep them, while those that prefer naked numbers can remove the parens.

Remove parentheses.
Replace hyphen(-) with minus(−).
Make font monospace.

I would say from top to bottom represents a diminishing return. Even just removing the parens already improves the UI signficantly for me, as my eyes can more easily see the leading character.

If it's impractical to replace all in one go, we can do it in steps. Starting with the (possibly optional) removal of the parens.

@mOctave
Copy link
Contributor

mOctave commented May 10, 2024

Maybe put in as an option? So those that prefer the parens can keep them, while those that prefer naked numbers can remove the parens.

I'd support a setting.

@TomGoodIdea TomGoodIdea added the waiting on OP The OP needs to provide something, e.g, making requested changes, posting assets, etc. label May 26, 2024
@Just-Existing Just-Existing marked this pull request as ready for review June 20, 2024 14:54
@TomGoodIdea TomGoodIdea removed the waiting on OP The OP needs to provide something, e.g, making requested changes, posting assets, etc. label Jun 20, 2024
source/MapDetailPanel.cpp Outdated Show resolved Hide resolved
source/MapDetailPanel.cpp Outdated Show resolved Hide resolved
source/Preferences.h Outdated Show resolved Hide resolved
source/PreferencesPanel.cpp Outdated Show resolved Hide resolved
source/MapDetailPanel.cpp Outdated Show resolved Hide resolved
source/Preferences.cpp Outdated Show resolved Hide resolved
source/Preferences.cpp Outdated Show resolved Hide resolved
source/Preferences.h Outdated Show resolved Hide resolved
source/Preferences.cpp Outdated Show resolved Hide resolved
Just-Existing and others added 3 commits June 20, 2024 19:02
Removed code that is useless until someone fixes the characters.
Co-authored-by: TomGoodIdea <108272452+TomGoodIdea@users.noreply.github.com>
@TomGoodIdea TomGoodIdea added the waiting on OP The OP needs to provide something, e.g, making requested changes, posting assets, etc. label Jul 25, 2024
@TomGoodIdea
Copy link
Member

@Just-Existing can you finish this?

@Just-Existing
Copy link
Contributor Author

No. I have no motivation to learn how to create tooltips in this codebase.

@TomGoodIdea
Copy link
Member

TomGoodIdea commented Aug 17, 2024

Just add an entry to the settings section in data/_ui/tooltips.txt. It's plain text, no coding required.
If you still don't want to do this, please close the PR.

@Just-Existing
Copy link
Contributor Author

Just add an entry to the settings section in data/_ui/tooltips.txt.

Done. Thanks for the help.

@tibetiroka tibetiroka added waiting on reviewer A Reviewer/Asignee needs to do something, e.g. reviewing, making suggestions, etc. and removed waiting on OP The OP needs to provide something, e.g, making requested changes, posting assets, etc. labels Aug 17, 2024
@TomGoodIdea TomGoodIdea added waiting on dev A developer needs to do something, e.g. reviewing, merging, resolving disputes, etc. and removed waiting on reviewer A Reviewer/Asignee needs to do something, e.g. reviewing, making suggestions, etc. labels Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI Everything related to the User Interface & Design (both artwork and code) waiting on dev A developer needs to do something, e.g. reviewing, merging, resolving disputes, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants