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.

Sign up for GitHub

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

Jump to bottom

Enable details screen for user's notes #242

Merged
merged 11 commits into from
Nov 28, 2020
Merged

Enable details screen for user's notes #242

merged 11 commits into from
Nov 28, 2020

Conversation

lukstbit
Copy link
Contributor

@lukstbit lukstbit commented Nov 22, 2020 •
edited
Loading

What does it fix?

Closes #86

Implements all the features from the linked issue. Some notes about the implementation:

  • To be able to actually show the files to the user, those need to be available so I removed the code to delete the files when the note upload is successful. If this code is merged, the user will not have any way to delete those files(besides deleting all the app data) so they'll remain on storage
  • To avoid complications, my code doesn't show a preview for uploaded videos. Instead I show a notice to the user and the option to see the video in their favorite app:

video_placeholder

I'm keeping it as draft to correct issues that I missed and get feedback.

How has it been tested?

Manually, tested it for various situations(only images, images and video, only videos etc)

@lukstbit lukstbit requested a review from aniri as a code owner November 22, 2020 13:15
@lukstbit lukstbit marked this pull request as draft November 22, 2020 13:15
@aniri
Copy link
Member

aniri commented Nov 23, 2020

@lukstbit There could be another way to show the sent notes pics and videos without keeping the files which could mean a lot of used storage :/ For the files that are successfully sent to the server, on the reply to the POST action, we receive the urls for the files. Maybe we could save them locally and load those if the file is no longer in local storage? If there is an internet connection, if not, show only the note text + a snackbar that no internet is available or a message instead of the picture. What do you think?

@lukstbit lukstbit force-pushed the implement_86 branch 2 times, most recently from 432a373 to bb47b3f Compare November 23, 2020 17:46
@lukstbit
Copy link
Contributor Author

What do you think?

Sounds great, I implemented this. I think the implementation is pretty ok and you can review it if you have time.

@lukstbit lukstbit marked this pull request as ready for review November 23, 2020 17:50
@aniri
Copy link
Member

aniri commented Nov 24, 2020

@lukstbit looking good! 🚀

Just a few more layout adjustments to be made:

  • add some margins to the images in the note details view
  • increase left/right margin of the text in the note details view
  • the forms no - question no text should look the same as the date text

Also, I noticed a weird small glitch: the forms no - question no value sometimes disappears, I think it does so after syncing notes with the server.

@lukstbit
Copy link
Contributor Author

I implemented the requested UI changes.

Also, I noticed a weird small glitch: the forms no - question no value sometimes disappears, I think it does so after syncing notes with the server.

I wasn't unable to reproduce this. However, while testing, I did notice a bug that prevented the NoteFragment and NoteFragmentDetails to show the correct question code(you can see the bug by going to forms -> selecting a form -> select a question(remember its code) -> swipe left/right to another question -> go to Add Note and add a note -> see that in the notes list the added note shows the code of the initial selected question and not the code of question the user swiped to). If you still see the bug you mentioned please let me know so I can take another look.

@aniri
Copy link
Member

aniri commented Nov 25, 2020 •
edited
Loading

hey @lukstbit it seems to crash now if there are no notes stored:

E/AndroidRuntime: FATAL EXCEPTION: main
    Process: ro.code4.monitorizarevot, PID: 31493
    java.lang.IllegalStateException: Parcels.unwrap<Question>…able(Constants.QUESTION)) must not be null
        at ro.code4.monitorizarevot.ui.notes.NoteFragment.onViewCreated(NoteFragment.kt:82)

@lukstbit
Copy link
Contributor Author

hey @lukstbit it seems to crash now if there are no notes stored:

I apologize for that, I added tens of notes from the questions list for testing and never once crossed my mind to add it from the option in the forms list. I should be fixed now.

At the moment the form/question codes string is built on the spot so they will not appear in the list show when you add notes from the forms list. If you want them to appear there as well I can store them in the database so they will be available everywhere.

@aniri
Copy link
Member

aniri commented Nov 26, 2020

At the moment the form/question codes string is built on the spot so they will not appear in the list show when you add notes from the forms list. If you want them to appear there as well I can store them in the database so they will be available everywhere.

@lukstbit It would be awesome if we could show them everywhere! As it's clearer for the users what they filled in :) But it is not as urgent, so we could make a new issue for this if you do not have time now.

And just one more small nitpick for this issue, could you also make the forms text the same style as the date text here:
Screenshot_20201126-231001_Monitorizare Vot

Thank you!! 🚀

@lukstbit
Copy link
Contributor Author

lukstbit commented Nov 28, 2020 •
edited
Loading

@aniri I implemented all of the requested changes, check it out!

@sonarqubecloud
Copy link

Please retry analysis of this Pull-Request directly on SonarCloud.

@aniri aniri merged commit 4d54abb into code4romania:develop Nov 28, 2020
@lukstbit lukstbit deleted the implement_86 branch November 28, 2020 12:22
aniri added a commit that referenced this pull request Dec 12, 2020
@aniri @DeKaN
@caldareanda @mrwhoknows55 @lukstbit @pedrofsn @AlexandraDaraban
Updates for december elections (#239)
ac79db5 
* fix send note on question

* Add simple usage for values from remote config at "About page"

* Use values from remote config for menu

* Copy files to cache instead of using directly. Fixes #178

* Remove file from cache after sending in any sync case

* Fix null string property

* Made font for selected item in the menu bold (#195)

* added collapse keyboard function (#215)

* added collapse keyboard function

* fix: keyboard collapse and opens again while switching multiple inputs

* refactor: collapseKeyboardIfFocusOutsideEditText() to Utils.kt

* Use the orderNumber field for sorting (#222)

* Use the orderNumber field for sorting

* Remove duplication for sonar

* Fix sorting for forms (#227)

* Sending firebase token on the login call (#225)

* Use filesDir instead of cacheDir (#230)

* Enable upload of multiple files with note (#231)

* Enable uploading multiple files with note

* Allow user to select multiple files

* Improve error strings for note files

* Update app/src/main/res/values-ro-rRO/strings.xml

Co-authored-by: Irina Borozan <anirib@gmail.com>

* replace: `this` with appropriate lifecycle owner (#224)

Co-authored-by: Irina Borozan <anirib@gmail.com>

* check db reset remote config flag on startup

* cleanups and add default value for remote config param

* Add sync related UI changes (#233)

* Add sync related UI changes

* small layout adjustments

* Add latest changes

Co-authored-by: Irina Borozan <anirib@gmail.com>

* removed extra activate

* Allow user to delete note files (#237)

* Allow user to delete note files

* Handle file deletion failure

* Send count of unsynced items to analytics (#244)

* Send count of unsynced items to analytics

* renamed variable to keep it consistent

Co-authored-by: Irina Borozan <anirib@gmail.com>

* fixed remote config init error and add min interval setting (#245)

* Fix RxJava null values usage (#240)

* Fix incorrect selection of counties (#247)

* Enable screen for visited polling stations (#241)

* Enable screen for visited polling stations

* Enable selecting from previously visited stations

* added visited stations icon from figma

* Implement requested changes

* Hide station selection on first run

Co-authored-by: Irina Borozan <anirib@gmail.com>

* Enable details screen for user's notes (#242)

* Enable details screen for user's notes

* Implement UI requested changes

* Fix issues with form-question codes

* Fix adding notes directly from forms list

* Tweak UI and save codes for note into database

* Update app/src/main/java/ro/code4/monitorizarevot/ui/notes/NoteDetailsFragment.kt

* Update app/src/main/res/values/strings.xml

* Apply suggestions from code review

Co-authored-by: Irina Borozan <anirib@gmail.com>

* Add info for visited stations screen (#256)

* Fix UI for notes after changing stations (#254)

* Bugfix/cleanups on app update (#257)

* refactored has selected stations shared pref

* fixed db cleanups after app update

* Add checks for empty answers list to sync (#252)

* added new link in menu and new guide (#259)

* changed logout display in menu and default safety link (#260)

Co-authored-by: Dmitriy <dekanszn@gmail.com>
Co-authored-by: caldareanda <61616802+caldareanda@users.noreply.github.com>
Co-authored-by: Avadhut Tanugade <30384908+mrwhoknows55@users.noreply.github.com>
Co-authored-by: lukstbit <52494258+lukstbit@users.noreply.github.com>
Co-authored-by: Pedro Francisco de Sousa Neto <pedrokra@gmail.com>
Co-authored-by: AlexandraDaraban <AlexandraDaraban@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aniri aniri aniri approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Enhancement] Improve the list of sent notes
2 participants
@lukstbit @aniri