Explore users #972

Merged
mmarif merged 13 commits from explore-users into main 2 weeks ago
mmarif commented 2 months ago
Owner

Closes #960

Closes #960
mmarif added this to the 4.1.0 milestone 2 months ago
mmarif added the
Feature
label 2 months ago
mmarif self-assigned this 2 months ago
mmarif added 1 commit 2 months ago
mmarif changed title from [WIP] Eplore users to [WIP] Explore users 2 months ago
mmarif added 1 commit 2 months ago
mmarif added 1 commit 2 months ago
mmarif added 1 commit 4 weeks ago
mmarif added 1 commit 4 weeks ago
mmarif added 1 commit 4 weeks ago
mmarif added 1 commit 4 weeks ago
mmarif changed title from [WIP] Explore users to Explore users 4 weeks ago
Poster
Owner

It's ready to test and review.

It's ready to test and review.
qwerty287 requested changes 4 weeks ago
<string name="navAbout">About</string>
<string name="navRate">Rate GitNex</string>
<string name="navLogout">Logout</string>
<string name="navExplore">Explore</string>
Poster

I don't know enough about Crowdin, but is this key deleted in the other locales when updating the translations via Crowdin? If not, it may be good to remove it everywhere.

I don't know enough about Crowdin, but is this key deleted in the other locales when updating the translations via Crowdin? If not, it may be good to remove it everywhere.
mmarif commented 4 weeks ago
Poster
Owner

Yes, it will be deleted automatically.

Yes, it will be deleted automatically.
qwerty287 marked this conversation as resolved
qwerty287 requested changes 4 weeks ago
TinyDB tinyDb = TinyDB.getInstance(context);
// if gitea is 1.12 or higher use the new limit
Poster

Sorry, forgot this question: do we need version checks for versions that are not supported by GitNex anymore? (This is more a general thing and could be changed in the hole app.)

Sorry, forgot this question: do we need version checks for versions that are not supported by GitNex anymore? (This is more a general thing and could be changed in the hole app.)
mmarif commented 4 weeks ago
Poster
Owner

There will be some installations which maybe are on old versions, so to be on the safe side we still need it for now. Maybe few more versions of Gitea down the road, we will remove this.

There will be some installations which maybe are on old versions, so to be on the safe side we still need it for now. Maybe few more versions of Gitea down the road, we will remove this.
qwerty287 marked this conversation as resolved
mmarif added 1 commit 4 weeks ago
mmarif added 1 commit 3 weeks ago
qwerty287 reviewed 3 weeks ago
style="@style/Widget.MaterialComponents.LinearProgressIndicator"
app:indicatorColor="?attr/progressIndicatorColor" />
<com.google.android.material.progressindicator.LinearProgressIndicator
Poster

Is it necessacy to have two linear progress indicators? Why not merging them into one? (Especially, since they are exactly the same)

Is it necessacy to have two linear progress indicators? Why not merging them into one? (Especially, since they are exactly the same)
mmarif commented 3 weeks ago
Poster
Owner

You have raised a very good point. Thanks for pointing it out, indeed they could be removed and just use one.

You have raised a very good point. Thanks for pointing it out, indeed they could be removed and just use one.
qwerty287 marked this conversation as resolved
mmarif added 1 commit 3 weeks ago
6791c9758d Merge branch 'main' into explore-users
mmarif added 1 commit 3 weeks ago
mmarif added 1 commit 3 weeks ago
qwerty287 requested changes 3 weeks ago
<!-- Notifications -->
<string name="pageTitleNotifications">Notifications</string>
<string name="noDataNotifications">No notifications found</string>
<string name="noDataNotifications">All caught up 🚀</string>
Poster

I like this idea. My only concern for the notifications string would be that it could be a bit strange if you view read notifications and there is the alert "All caught up". Should be changed.

I like this idea. My only concern for the notifications string would be that it could be a bit strange if you view read notifications and there is the alert "All caught up". Should be changed.
mmarif commented 3 weeks ago
Poster
Owner

If you have no notifications either read/unread it automatically means caught up, means you are good.

In rare case someone would may have no notifications for read, in that scenario we can extend the string though.

I just put an idea for another release because currently all the strings are boring.

If you have no notifications either read/unread it automatically means `caught up`, means you are good. In rare case someone would may have no notifications for read, in that scenario we can extend the string though. I just put an idea for another release because currently all the strings are boring.
Poster

In rare case someone would may have no notifications for read, in that scenario we can extend the string though.

This is exactly what I mean 🙃

> In rare case someone would may have no notifications for read, in that scenario we can extend the string though. This is exactly what I mean 🙃
mmarif commented 3 weeks ago
Poster
Owner

Yes, I know. We will dig more in the next release.

Yes, I know. We will dig more in the next release.
qwerty287 marked this conversation as resolved
mmarif added 1 commit 3 weeks ago

Sorry, I'm currently on my phone and the diff is too big to load - maybe we should add the review feature😄 (especially since there is already one that comments on the issue - maybe I'll look into this), but I approve this PR if it's working like this too - if not you have to wait until I can approve it on my computer

Sorry, I'm currently on my phone and the diff is too big to load - maybe we should add the review feature😄 (especially since there is already one that comments on the issue - maybe I'll look into this), but I approve this PR if it's working like this too - if not you have to wait until I can approve it on my computer
Poster
Owner

No problem. You can approve it later. I will merge it afterwards.

No problem. You can approve it later. I will merge it afterwards.
qwerty287 approved these changes 2 weeks ago
mmarif merged commit 22cd23525e into main 2 weeks ago
mmarif referenced this issue from a commit 2 weeks ago
mmarif deleted branch explore-users 2 weeks ago

Reviewers

qwerty287 approved these changes 2 weeks ago
The pull request has been merged as 22cd23525e.
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.