neffo / bing-wallpaper-gnome-extension

GNOME shell extension that syncs your desktop & lock screen wallpaper to Microsoft Bing's Image of the Day.
https://extensions.gnome.org/extension/1262/bing-wallpaper-changer/
GNU General Public License v3.0
299 stars 56 forks source link

UI Improvements #252

Closed Wartybix closed 2 weeks ago

Wartybix commented 2 weeks ago

Hello,

Firstly, thank you for creating this extension. It is one of my favourites, and it's nice to see a lovely new background every day on my desktop.

I've made some changes to the UI, mainly affecting the preferences window. In each of my commits, I've made a particular change, and have justified why in the commit message details. Overall, the changes are to make the UI more in line with GNOME's Human Interface Guidelines, making it easier to use. Notable changes are:

Attached are screenshots of the preferences window with the improvements: Screenshot From 2024-11-07 08-18-40 Screenshot From 2024-11-07 08-18-45 Screenshot From 2024-11-07 08-18-51 Screenshot From 2024-11-07 08-18-55 Screenshot From 2024-11-07 08-19-04

neffo commented 2 weeks ago

Wow this is amazing, thanks for your effort. I'm happy to merge this into the next version (51).

Is it possible you can base it off this branch? https://github.com/neffo/bing-wallpaper-gnome-extension/tree/version-51

I've done this by adding a new English (en) translation file with these modifications, rather than changing the strings in the source code, so it doesn't break and 'un-translate' other translations.

I think we could just bulk find & replace this in all the translations and in the source if you'd like. That is how I've made these changes in the past, although maybe not to the same degree. We rebuild all the compiled translations at build anyway.

This is the only comment I have on these changes other than I agree. I've not across the HIGs but of course would prefer the extension follows it.

Did you have further changes in mind with this PR?

Wartybix commented 2 weeks ago

Okay, I have submitted a new pull request #253, based off of the version-51 branch 😁. There are some conflicts though, so do you want me to make some changes on my end, or will you resolve them? I suppose it makes more sense to talk about that in the new PR's comment thread though.

I think we could just bulk find & replace this in all the translations and in the source if you'd like.

That sounds good -- it's up to you though. I just made the text style changes as a new 'en' translation to try to keep the amount of code review and extra work for this small (arguably nit-picky) change as low as possible. Functionally it should work the same, using either the new 'en' translation, or using your method of find-and-replacing everything. Your idea does have the benefit of making it easier for future translators, as changing the message IDs to use heading styles etc. tells the translators that translations should also have those same styles.

I don't think I have anything else to add to the PR unless you want me to make some modifications. I was actually using this extension as a reference while making my first GNOME extension (called Auto Accent Colour, which I've recently submitted to the extension store for review and works quite nicely alongside yours, as it automatically sets your GNOME 47+ accent colour from colours in your wallpaper). Anyway, once I uploaded my extension, I just thought it would be fun to make some HIG-conforming improvements to yours after spending time looking at your source code and preferences window.