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

Update note card and other CSS #247

Merged
merged 16 commits into from
Oct 27, 2020

Conversation

qwoprocks
Copy link
Member

@qwoprocks qwoprocks commented Oct 27, 2020

Description

Fixes #217, fixes #250

Changes (view ss below for reference):

  • Added proper note card (supports tags) to country note list panel
  • Changed the menuitems on the top bar so that the text can be seen (previously was white text on grey/white bg)
  • Refactored country note list panel to use a ScollPane, and a VBox inside it containing all the notes that updates using a observablelist listener
  • Made it so that Tags can only be max 45 characters long

image

Testing

Ran all test, passed all tests.
Ran the following commands:
country note delete 2
country note add c/US nt/blahblahabalmecor ncomn
country note view c/US
country note delete 1
client edit 1 t/pneumonoultramicroscopicsilicovolcanoconiosis (success)
client edit 1 t/pneumonoultramicroscopicsilicovolcanoconiosism (fail)
client add n/mmm p/901 e/m@c.com a/bac,lem c/SG tz/GMT+9 t/pneumonoultramicroscopicsilicovolcanoconiosis t/pneumonoultramicroscopicsilicovolcanoconiosimmm (fail)
client add n/mmm p/901 e/m@c.com a/bac,lem c/SG tz/GMT+9 t/pneumonoultramicroscopicsilicovolcanoconiosis t/pneumonoultramicroscopicsilicovolcanoconios (success)

Remarks

Not sure if we want to use the same way for clients as I did for notes (refer to point 3 of changes). @tankangliang @raysonkoh @rtshkmr @LeeEnHao what do y'all think? The current listview has buggy behavior with scrolling, if you have one cell that's much longer than the others, the scrollbar gets smaller when that cell is rendered, and then bigger again as you scroll on. Its most obvious when you click and drag the scrollbar.

@qwoprocks qwoprocks added type.Task Something that needs to be done, but not a story, bug, or an epic. e.g. Move testing code into a new priority.High Must do labels Oct 27, 2020
@qwoprocks qwoprocks added this to the v1.3 milestone Oct 27, 2020
@qwoprocks qwoprocks self-assigned this Oct 27, 2020
@qwoprocks qwoprocks added this to In progress in v1.3 via automation Oct 27, 2020
@codecov
Copy link

codecov bot commented Oct 27, 2020

Codecov Report

Merging #247 into master will decrease coverage by 0.16%.
The diff coverage is 76.82%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #247      +/-   ##
============================================
- Coverage     84.61%   84.44%   -0.17%     
- Complexity      851      864      +13     
============================================
  Files           110      110              
  Lines          2320     2372      +52     
  Branches        255      263       +8     
============================================
+ Hits           1963     2003      +40     
- Misses          271      280       +9     
- Partials         86       89       +3     
Impacted Files Coverage Δ Complexity Δ
...in/java/seedu/address/ui/CountryNoteListPanel.java 55.76% <57.77%> (+24.51%) 5.00 <5.00> (+4.00)
...ress/logic/parser/CountryNoteAddCommandParser.java 100.00% <100.00%> (ø) 5.00 <0.00> (ø)
...edu/address/model/country/CountryNotesManager.java 73.91% <100.00%> (+5.49%) 8.00 <0.00> (+1.00)
...ain/java/seedu/address/model/note/CountryNote.java 91.66% <100.00%> (+0.75%) 7.00 <2.00> (+1.00)
src/main/java/seedu/address/model/note/Note.java 100.00% <100.00%> (ø) 13.00 <3.00> (ø)
src/main/java/seedu/address/model/tag/Tag.java 90.90% <100.00%> (ø) 10.00 <3.00> (+2.00)
...in/java/seedu/address/storage/JsonAdaptedNote.java 91.66% <100.00%> (ø) 6.00 <0.00> (ø)
src/main/java/seedu/address/ui/ClientListCard.java 96.00% <100.00%> (ø) 8.00 <0.00> (?)
...rc/main/java/seedu/address/ui/ClientListPanel.java 90.90% <100.00%> (-0.76%) 2.00 <0.00> (ø)
src/main/java/seedu/address/ui/NoteListCard.java 100.00% <100.00%> (ø) 6.00 <6.00> (?)
... and 1 more

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 a357839...c88760c. Read the comment docs.

Copy link

@raysonkoh raysonkoh left a comment

Choose a reason for hiding this comment

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

Manually tested GUI, LGTM.

v1.3 automation moved this from In progress to Reviewer approved Oct 27, 2020
@@ -9,7 +9,9 @@
*/
public class Tag {

public static final String MESSAGE_CONSTRAINTS = "Tags names should be alphanumeric";
public static final int MAX_CHARACTERS = 45;
public static final String MESSAGE_CONSTRAINTS = "Tags names should be alphanumeric and be within " + MAX_CHARACTERS

Choose a reason for hiding this comment

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

Maybe rephrase to "maximum of 45 characters" instead of "be within 45 characters"?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@FXML
private Label header;
@FXML
private ListView<CountryNote> countryNoteListView;
private VBox countryNoteListView;

Choose a reason for hiding this comment

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

Should this be countryListView since the subpanel contains a countryNoteListView?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is being used to display country notes if we're looking at one country only, so I just left it as countryNoteListView

v1.3 automation moved this from Reviewer approved to Review in progress Oct 27, 2020
Copy link
Member

@LeeEnHao LeeEnHao left a comment

Choose a reason for hiding this comment

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

Looks good. There's still a slight aesthetic issue where the list cell is not contained within the list view rounded container, but this is pretty trivial.

image

@qwoprocks
Copy link
Member Author

Looks good. There's still a slight aesthetic issue where the list cell is not contained within the list view rounded container, but this is pretty trivial.

image

good catch, I'll add it as an issue

v1.3 automation moved this from Review in progress to Reviewer approved Oct 27, 2020
@qwoprocks qwoprocks merged commit c1beeb1 into AY2021S1-CS2103T-F11-4:master Oct 27, 2020
v1.3 automation moved this from Reviewer approved to Done Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority.High Must do type.Task Something that needs to be done, but not a story, bug, or an epic. e.g. Move testing code into a new
Projects
No open projects
v1.3
Done
Development

Successfully merging this pull request may close these issues.

Fix client card display issue Beautify country notes card
4 participants