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

Add about #18

Merged
merged 2 commits into from
Apr 15, 2023
Merged

Add about #18

merged 2 commits into from
Apr 15, 2023

Conversation

niccokunzmann
Copy link
Member

This fixes #16.

image

@dbrgn
Copy link

dbrgn commented Sep 3, 2022

Hi, thanks for the PR. I have another idea, how this would be solved better: I wouldn't look for the "about" information in the settings. Instead of showing the three fixed icons in the top right, add a menu to the action bar. In src/res/menu/main.xml:

    <item
        android:id="@+id/menu_about"
        android:icon="@drawable/ic_TODO"
        android:showAsAction="never"
        android:title="@string/btn_about"/>

When the user taps on this menu entry, a dedicated about activity is shown. The license information should also be part of that about screen.

screenshot-20220903-120922

screenshot-20220903-120932

What do you think? Would you like to implement this?

@dbrgn dbrgn mentioned this pull request Sep 3, 2022
3 tasks
@niccokunzmann
Copy link
Member Author

I am happy to see a menu. I do not know when I will get to this or who will. The code is quite modular and it can be moved. Personally, I would merge it and create a new issue to create a menu - this change is better than before but not perfect - in my eyes. It might be that this fix will not be shipped for a while if it is not merged. But that is also your decision as a maintainer.

My maintenance style would be hopefully "Done is better than perfect" & "small steps with fast feedback" and for community: "engaged and merged (and reverted) is better than rejected/stale" - but they have to be weighted with long-term maintainability - so your choice as the maintainer!

I am too tired to do it now properly.

- Version information
- License
- Repository URL and button to visit it
@dbrgn
Copy link

dbrgn commented Apr 15, 2023

There are a few issues with this PR:

  • Layout: The left alignment of the menu entry is incorrect
  • Layout nitpick: Missing space after the colons
  • A lot of unused imports
  • At least on my test device (Emulator with API 31), the button pointing to the repository doesn't work

A lot of this is due to the approach of putting content into the settings view, which isn't made for that. This results in things like having to write a custom layout, just to wrap the content.

As for the button, I'm not sure if the approach of just putting the URL into an intent is valid at all. It probably depends on the Android version, whether this works or not. A much easier approach is simply making the URL in the text view clickable.

I rebased your branch on top of current main, removed unrelated changes (like the Gradle update) and will fix some of the issues above directly in this branch. The move to a separate activity can then be done in separate MRs.

- Button doesn't seem to work on API 31, remove it and replace it with
  linked URL instead
- Fix a few layout bugs
- Remove unused imports
@dbrgn
Copy link

dbrgn commented Apr 15, 2023

Doesn't look too bad now!

image

@dbrgn dbrgn merged commit 978c022 into spaceapi-community:main Apr 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

About Activity
2 participants