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

WebUI: Show country/region name next to its flag when 'Resolve peer countries' is enabled #21278

Merged
merged 3 commits into from
Sep 6, 2024

Conversation

skomerko
Copy link
Contributor

Example:

image

src/webui/www/private/scripts/dynamicTable.js Outdated Show resolved Hide resolved
"title": country
}));
}
span.style.backgroundImage = `url('images/flags/${!country_code ? "xx" : country_code}.svg')`;
Copy link
Member

Choose a reason for hiding this comment

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

Using "xx" seems suspicious. I suppose you should either set backgroundImage= "none"; or remove the property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It probably could have been named better but xx.svg is actual icon that can be used to represent unknown country: https://github.com/qbittorrent/qBittorrent/blob/master/src/icons/flags/xx.svg

I thought it's better to use it than just display nothing at all.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I wasn't familiar with xx. https://flagicons.lipis.dev/ shows it as 'Unknown'. We're good here.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose we can add a comment about xx represents 'Unknown'.

@Chocobo1 Chocobo1 added the WebUI WebUI-related issues/changes label Aug 31, 2024
@Chocobo1 Chocobo1 added this to the 5.1 milestone Aug 31, 2024
Co-authored-by: Chocobo1 <Chocobo1@users.noreply.github.com>
@Chocobo1 Chocobo1 requested a review from a team September 1, 2024 06:58
Co-authored-by: Chocobo1 <Chocobo1@users.noreply.github.com>
@thalieht
Copy link
Contributor

thalieht commented Sep 1, 2024

I hope it gets accepted #3408 (comment)

@Chocobo1
Copy link
Member

Chocobo1 commented Sep 2, 2024

I hope it gets accepted #3408 (comment)

@thalieht @skomerko
I don't have strong opinion about it. I don't mind you submit a PR for GUI that follows what webui has done in this PR.

@glassez
Copy link
Member

glassez commented Sep 2, 2024

I hope it gets accepted #3408 (comment)

I'm OK unless you're going to add a separate column for it.

@thalieht
Copy link
Contributor

thalieht commented Sep 2, 2024

I probably didn't say it right. I meant i hope this PR gets accepted because the maintainer rejected the idea in the past.

@xavier2k6
Copy link
Member

Having an extra/separate column got rejected, showing "name" like this PR in GUI would be most welcome/acceptable as it's done in same/pre-existing column.

@stalkerok
Copy link
Contributor

The current list of peers in the GUI only shows the country flag, the country name is only shown in the tooltip, wouldn't it be better to make this behavior the same?

@Chocobo1
Copy link
Member

Chocobo1 commented Sep 3, 2024

The current list of peers in the GUI only shows the country flag, the country name is only shown in the tooltip, wouldn't it be better to make this behavior the same?

Yes. That is what I meant. GUI and webui are now considered separate entities so it is not required to be done in the same PR IMO.

@glassez
Copy link
Member

glassez commented Sep 3, 2024

GUI and webui are now considered separate entities so it is not required to be done in the same PR IMO.

👍

@stalkerok
Copy link
Contributor

Yes. That is what I meant. GUI and webui are now considered separate entities so it is not required to be done in the same PR IMO.

I agree. It would be nice to keep the same behavior for GUI and WebUI, maybe even separate the peer country flag and country name. That would also close the feature request for this. (could be done in a different PR)

@Chocobo1 Chocobo1 merged commit f681e95 into qbittorrent:master Sep 6, 2024
14 checks passed
@Chocobo1
Copy link
Member

Chocobo1 commented Sep 6, 2024

@skomerko
Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebUI WebUI-related issues/changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants