search

SankethBK / diaryvault

A personal diary application written in Flutter
https://play.google.com/store/apps/details?id=me.sankethbk.dairyapp
MIT License
86 stars 59 forks source link

Refactored & fixed NoteRead feature #65

Closed vaibhavppandey closed 11 months ago

vaibhavppandey commented 11 months ago

What type of PR is this? (check all applicable)

Description

refactored the code made by the prev contributor and fixed minor bugs

Features Covered in this PR

Related Tickets & Documents

Screenshots, Recordings

https://github.com/SankethBK/diaryvault/assets/113239561/a4d5aad1-2961-4a49-bff9-ce07c3e5c348

Tested Feature??

vaibhavppandey commented 11 months ago

Also @SankethBK, I was planning to implement the bloc arch for state management for this feat. But given the feat's current scope , I went with the simple setState . But I am ready to implement the bloc arch if you say so.

SankethBK commented 11 months ago

@vaibhavppandey setState is sufficient for this, no need for bloc as the state can be contained within the widget

SankethBK commented 11 months ago

Minor comments, rest all looks good! Nice job!!

micedreams commented 11 months ago

@vaibhavppandey setState is sufficient for this, no need for bloc as the state can be contained within the widget

@SankethBK I just saw this... please take it back!!😂😂
SetState is never sufficient... If you don't agree with me this might help change your mind...

SankethBK commented 11 months ago

@vaibhavppandey setState is sufficient for this, no need for bloc as the state can be contained within the widget

@SankethBK I just saw this... please take it back!!😂😂 SetState is never sufficient... If you don't agree with me this might help change your mind...

I agree, it is definitely not a good practice to call setState on a widget that has large number of children widgets. But since this widget doesn't has any child widget, i think it should be fine

vaibhavppandey commented 11 months ago

@vaibhavppandey setState is sufficient for this, no need for bloc as the state can be contained within the widget

@SankethBK I just saw this... please take it back!!😂😂
SetState is never sufficient... If you don't agree with me this might help change your mind...

Ya I second this but as @SankethBK mentioned, the NoteReadIconButton isn't complex yet so I used setState. However, based on the feedback , I've transitioned toValueNotifier. Thank you for the review! Got to learn something new

SankethBK commented 11 months ago

@micedreams I don't see any new linter errors, once we fix the ones in master we'll be able to track it easily.