Upload answered quests #696
Conversation
There was a problem hiding this comment.
Looks good! I've added some comments with remarks. Also, you should probably correct the indentation in the lambdas.
Note that I am not sure if I can squeeze it into v3, because I will be away for the whole of December and want to minimize the risk of bugs popping in that time (= not introduce any new features)
| import de.westnordost.streetcomplete.data.QuestStatus; | ||
| import de.westnordost.streetcomplete.data.osm.persist.OsmQuestDao; | ||
|
|
||
| public class UploadsCounter extends android.support.v7.widget.AppCompatTextView { |
There was a problem hiding this comment.
The name for the new class is confusing. After all, this is the counter of answers that have not been uploaded yet.
The other (original) counter counts the number of quest answers that have been uploaded already. This one counts the number of newly added quest answers that are pending to be uploaded. The class and field names should reflect that.
There was a problem hiding this comment.
Yeah, I did feel there was something off. How about if I renamed both the classes. UnsyncedAnswersCounter and UploadedAnswersCounter? Both their field names can be answeredQuests since the class name will specify what kind of answered quests they contain.
There was a problem hiding this comment.
These names would work, yes.
answeredQuests
I meant the names of the fields in MainActivity.
| <de.westnordost.streetcomplete.statistics.AnswersCounter | ||
| android:id="@+id/answersCounter" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_gravity="end" | ||
| android:minEms="7" | ||
| android:paddingEnd="8dp" | ||
| android:paddingStart="8dp" |
There was a problem hiding this comment.
Wouldn't that look very packed on smaller devices? Does it even fit with four-digit counters each?
I think it is not really necessary to display the app's name in the header. This way, there will be more than enough space.
There was a problem hiding this comment.
Removed the title and moved the counters to the left. I added back the padding. The minEms is adding a lot of space so not sure if its needed. The text being dynamic seems to make enough space for itself and the padding ensures they are not squished. Tested the layout on on a pixel emulator.
| android:drawablePadding="2dp" | ||
| android:drawableStart="@drawable/ic_file_upload_24dp" | ||
| android:gravity="center_vertical" | ||
| /> |
There was a problem hiding this comment.
The component should have a proper selectable item background because it is clickable.
There was a problem hiding this comment.
I added 'android:background="?android:attr/selectableItemBackground"' to the component. Assuming that's what you had in mind.
Seperated logic out of Answers Counter to represent answers to quests that haven't been uploaded.
…ft hand side of the toolbar
errolpais
force-pushed
the
UploadAnsweredQuests
branch
from
e481c4b to
710223c
Compare
|
|
Yay! Thanks for the feedback and for maintaining the app 👍 |
Hi @westnordost,
This addresses issue #75. I added a new upload icon into the app bar. I tested it manually prior to the java 8 upgrade and it looked good. I just re based my commits for this pull request. The app deploys and looks alright on my phone, but will test it out manually tomorrow.
I wanted to get some feedback on the pull request. I did pull out the ems configuration to make things fit and it seems alright but I might be missing something, Please let me know what you think and if you'd like me to make any additions/changes.
Thanks