Skip to content

Conversation

@HM201
Copy link

@HM201 HM201 commented Dec 22, 2018

I added a fastscrollscrollview in the notes layout upon request on issue #523
I made some layout changes in the notes layout based on user request on issue #434

…lso added some changes in note layout upon user request
@federicoiosue
Copy link
Owner

Hi Misham,

thanks for your contribution. However there are some problems to be solved before merging your pull request:

  1. Your code misses both UI and unit testing.

  2. The first reason of missing tests is that the code has been took from this library so I'm asking: why copying code (that would also increase technical debt of Omni Notes) instead of using that as external library? Yes that library does not contains tests neither but eventual changes or fixes made to it would have been easier to include into ON later.

  3. The fastscroll code/layout modification causes a problem with the position of the scrollbar itself, as shown into this screenshot
    12

  4. The note's detail space layout modification causes the following whenever a note is going to be modified, except than when is is long enough to be scrolled vertically

android_emulator_-_nexus_5x_api_28_x86_5554

Great work for now, I've tried and this change really seems useful!

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.

2 participants