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

Refactored & fixed NoteRead feature #65

Merged
merged 5 commits into from
Oct 7, 2023
Merged

Refactored & fixed NoteRead feature #65

merged 5 commits into from
Oct 7, 2023

Conversation

vaibhavppandey
Copy link
Contributor

@vaibhavppandey vaibhavppandey commented Oct 6, 2023 •
edited
Loading

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

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

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

Features Covered in this PR

  • made a different NoteReadIconButton widget to seperate TTS feat
  • fixed bug, where the icon changes to indicate play when TTS completion happens

Related Tickets & Documents

Screenshots, Recordings

out.mp4

Tested Feature??

  • In Real Device.
  • In Emulator

Night-Amber3301 and others added 2 commits October 5, 2023 14:43
@Night-Amber3301
Updated the file by adding pause button functionality and created rea…
db38951 
…d_notes.dart in widgets directory. (SankethBK#62)

* Update note_read_only_page.dart

* Create read_notes.dart
@vaibhavppandey
fix: note read feat
2882f60
@vaibhavppandey
Copy link
Contributor Author

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
Copy link
Owner

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

@SankethBK SankethBK changed the base branch from feat/read_notes to master October 6, 2023 21:20
@SankethBK
Copy link
Owner

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

micedreams
micedreams reviewed Oct 6, 2023
View reviewed changes
Copy link
Collaborator

@micedreams micedreams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your PR looks pretty good except for a few things...good job!!

@micedreams
Copy link
Collaborator

micedreams commented Oct 6, 2023 •
edited
Loading

@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
Copy link
Owner

@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
Copy link
Contributor Author

@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
SankethBK approved these changes Oct 7, 2023
View reviewed changes
Copy link
Owner

@SankethBK SankethBK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for contributing!

@SankethBK
Copy link
Owner

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

@SankethBK SankethBK merged commit 6be55c1 into SankethBK:master Oct 7, 2023
@vaibhavppandey vaibhavppandey deleted the feat/read_notes branch October 7, 2023 12:36
SankethBK pushed a commit that referenced this pull request Oct 20, 2023
@vaibhavppandey @Night-Amber3301 @SankethBK
Refactored & fixed NoteRead feature (#65)
406f3cd 
* Updated the file by adding pause button functionality and created read_notes.dart in widgets directory. (#62)

---------

Co-authored-by: Pratibhanu Jarngal <[email protected]>
SankethBK pushed a commit that referenced this pull request Oct 20, 2023
@bibekpanda55 @Night-Amber3301 @SankethBK
Refactored & fixed NoteRead feature (#65)
8738e24 
* Updated the file by adding pause button functionality and created read_notes.dart in widgets directory. (#62)

---------

Co-authored-by: Pratibhanu Jarngal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@micedreams micedreams micedreams left review comments

@SankethBK SankethBK SankethBK 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.

Implement Text-to-Speech Feature for Note Content Reading
4 participants
@vaibhavppandey @SankethBK @micedreams @Night-Amber3301