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.

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

Integrate Notes feature with Ui #121

Merged
merged 15 commits into from
Oct 27, 2020

Conversation

TanLeYang
Copy link
Collaborator

Closes #112
Integrate the notes feature into the Ui.

For books that have notes added, the Ui will show the title of the notes in the summarized view.
Notes summarised

When viewing a book in the detailed view, the Ui will display the title and text of all the notes added to the book.

Notes detailed

The notes are displayed in the detailed view using JavaFx's ListView included in DetailedBookListPanel

@TanLeYang TanLeYang added the type.Story A user story label Oct 25, 2020
@TanLeYang TanLeYang added this to the v1.3 milestone Oct 25, 2020
@TanLeYang TanLeYang self-assigned this Oct 25, 2020
Copy link
Collaborator

@pangpuncake pangpuncake left a comment

Choose a reason for hiding this comment

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

LGTM! Great work!

@codecov-io
Copy link

Codecov Report

Merging #121 into master will decrease coverage by 1.30%.
The diff coverage is 2.38%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #121      +/-   ##
============================================
- Coverage     74.10%   72.80%   -1.31%     
  Complexity      669      669              
============================================
  Files           105      106       +1     
  Lines          2078     2114      +36     
  Branches        238      242       +4     
============================================
- Hits           1540     1539       -1     
- Misses          455      492      +37     
  Partials         83       83              
Impacted Files Coverage Δ Complexity Δ
...a/seedu/bookmark/logic/commands/CommandResult.java 87.50% <ø> (ø) 11.00 <0.00> (ø)
src/main/java/seedu/bookmark/model/book/Book.java 90.90% <0.00%> (-1.20%) 32.00 <0.00> (ø)
src/main/java/seedu/bookmark/ui/BookCard.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
src/main/java/seedu/bookmark/ui/BookListPanel.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
.../java/seedu/bookmark/ui/DetailedBookListPanel.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
src/main/java/seedu/bookmark/ui/MainWindow.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
src/main/java/seedu/bookmark/ui/NoteCard.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
.../seedu/bookmark/logic/commands/AddNoteCommand.java 94.28% <100.00%> (-0.16%) 11.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 917fbfe...73724a0. Read the comment docs.

Copy link
Collaborator

@pennhanlee pennhanlee left a comment

Choose a reason for hiding this comment

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

LGTM!!

Copy link
Collaborator

@angrybunny123 angrybunny123 left a comment

Choose a reason for hiding this comment

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

LGTM, just a question about the DG.

@@ -184,7 +185,7 @@ detailed view:

* **Alternative 1 (current choice):** Use JavaFX ListView
* Pros: Easy to keep UI up to sync with model by overriding ListCell's updateItem method
* Cons: Can allow for displaying of multiple DetailedBookCards even though the detailed view is only meant to show
* Cons: Technically allows for displaying of multiple books even though the detailed view is only meant to show
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think saying 'Technically Allows' is a bit weird because it implies that it is a bug. Maybe we can add a statement to explain why the bug wouldn't occur?

@TanLeYang TanLeYang merged commit 6366518 into AY2021S1-CS2103T-F13-2:master Oct 27, 2020
Copy link
Collaborator

@pennhanlee pennhanlee left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type.Story A user story
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate Note into UI
5 participants