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

Show fallback icon on missing country flag icon #2793

Merged
merged 1 commit into from Feb 3, 2017

Conversation

@Kissaki
Copy link
Member

commented Jan 30, 2017

If a country flag icon does not exist, we now display a placeholder (__.svg)

It looks like this, with the Aland Islands missing its flag.

2017-01-30 21_02_32-mumble server connect


  • Is the placeholder ok? Maybe make it less dark, and increase the question mark size?
  • I’m not too fond of adding the __.svg to the icons/flags folder, which is otherwise only SVGs from emojione, plus our own README and LICENSE files. But I don’t know where else I would put it, it fits best in there after all.

@Kissaki Kissaki added the ui label Jan 30, 2017

@Kissaki Kissaki requested a review from mkrautz Jan 30, 2017

@mkrautz

This comment has been minimized.

Copy link
Member

commented Jan 30, 2017

How about just using the globe icon? That's what I initially thought would be good.

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2017

I don't think that's a good idea. We use the globe icon for categories/continent, so I would rather not use it for countries.

Through its bright color, the globe is also very prominent and jumps into the eye compared to the country flags.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Jan 30, 2017

I honestly prefer the globe icon. It's an error state, it's not really meant to happen. It's an icon we already have, and it doesn't look too out of place for a country/region where we a flag is missing.

@hacst Thoughts?

@hacst

This comment has been minimized.

Copy link
Member

commented Jan 30, 2017

Don't have strong feelings in either direction. Personally I would've been fine with leaving it blank (did that produce visual artifacts?).

EDIT: Just saw the other issue. From my POV I would've been fine with the previous behavior. If you think we need a placeholder I guess I slightly prefer the globe one unless we can get a better made "?" one. It looks kinda off in the screenshot as is.

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2017

def-globe def-grey

none grey2 globe

Maybe grey one should be darker on dark skin (but that's a skin thing)
grey

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2017

Actually, wouldn’t it be nice to have continent icons for the continents?
Iso 3166 does not define them, and we (emojione) does not provide them. Maxmind defines the continent codes here. We already have the continent codes in our data. Sadly, no icons.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Feb 1, 2017

How about we try to figure out which flags we're actually missing and perhaps update mkflags.py (or add another script) to print the missing flags?

Perhaps our coverage is pretty good?

(Though, I would prefer just falling back to the globe either way.)

@mkrautz

This comment has been minimized.

Copy link
Member

commented Feb 1, 2017

(BTW, this PR also contains the retracted continent commit?)

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2017

Sorry, that’s not supposed to be there. I’ll remove that commit.

@Kissaki Kissaki force-pushed the Kissaki:flag-unknown branch from 4096e2a to f1d91ed Feb 2, 2017

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2017

The question mark placeholder could be misinterpreted as unknown - but the country (and probably its flag) is known, it's just that we do not have an icon.

The benefit of having some kind of icon is the name being vertically aligned with the others. Perhabs aligning it with a space rather than icon would work as well.

The globe is used for continents, undistinguishing. It already is sort of a placeholder (for this category type). Countries are a different level than continents though. All of these are subdivisions of the globe.

Let’s just use the globe then, to have something.

I expect us to have good coverage already. We have 257 flags, Maxmind lists 254 possible country codes, which includes stuff like the following which we do not have flags for (so the numbers are not direct matches; they can not even be represented in the corresponding countrycode a-z unicode):

A1,"Anonymous Proxy"
A2,"Satellite Provider"
O1,"Other Country"
Show fallback icon on missing country flag icon
If a country flag icon does not exist, we now display the globe icon, which
is being used for the continents as well.

This will vertically align the country name with the others that have an
icon.

@Kissaki Kissaki force-pushed the Kissaki:flag-unknown branch from f1d91ed to 95c7b5e Feb 2, 2017

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2017

I pushed the changes accordingly, to display the globe icon. Please review again.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

@Kissaki should the commit summary be worded differently now that we use the globe icon?

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Feb 3, 2017

I did change the commit message accordingly. Did I miss something?

The globe icon is used as a fallback icon. Second sentence should make that clear.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Feb 3, 2017

Ah, I might have confused it with the PR title.

@mkrautz mkrautz changed the title Add unknown country flag icon as fallback Show fallback icon on missing country flag icon Feb 3, 2017

@mkrautz
mkrautz approved these changes Feb 3, 2017

@mkrautz mkrautz merged commit d871f34 into mumble-voip:master Feb 3, 2017

@Kissaki Kissaki deleted the Kissaki:flag-unknown branch Feb 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.