Move licenses from window menu to settings screen#477
Move licenses from window menu to settings screen#477florent37 merged 3 commits intoopenflocon:mainfrom
Conversation
Summary of ChangesHello @timoloewe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the application's license information display by relocating it from the system-level window menu to a more integrated 'About' section within the in-app settings. This change aims to improve the user experience, particularly on Linux where the window menu was visually intrusive, and streamlines the application's overall UI by consolidating settings-related options into a single, consistent location. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully moves the licenses screen access from a window menu item to a button within the settings screen. This is a great improvement for Linux users as it saves vertical space and integrates better with the desktop environment, and it also consolidates settings into one place. The changes are well-implemented, removing the old menu and adding the new UI elements cleanly. I've left one minor suggestion to use a string resource for a hardcoded window title to improve maintainability.
| onCloseRequest: () -> Unit | ||
| ) { | ||
| FloconWindow( | ||
| title = "Licenses", |
There was a problem hiding this comment.
To improve maintainability and support for localization, it's best to use string resources instead of hardcoding text. You've already added a string resource for "Licenses", so it should be used here for the window title.
| title = "Licenses", | |
| title = stringResource(Res.string.settings_licenses), |
| .background(FloconTheme.colorPalette.primary), | ||
| ) | ||
| } | ||
|
|
|
Hi @timoloewe thanks for the feedback, yes I only tried on macos, and never faced this ux bad behavior then |
3 participants
Hey everyone! Thanks for this great project!
I'm a Linux user, and on Linux, Flocon's app window is rendered like this:
I'm not sure how this looks on other platforms (I guess it at least looks different on MacOS), but I don't like how the "Settings -> Licenses" menu is permanently rendered below the window title bar:
I was wondering if you would accept a change that would get rid of the window menu and instead moves the "Licenses" item to the in-app settings screen. That would also get rid of the fact that there are currently two places that are labeled as "Settings" (window menu and in-app).
Here's a screenshot of what my proposal looks like:
Of course, I'm happy to change visuals or other details according to your feedback. Also, feel free to reject if you prefer to keep the current state.