Ui: Add colors to tag labels #798

yamgent · 2018-01-19T05:32:31Z

Part of #784. Rebased, and changed the design of determining the colours of the tag.

Propsed commit message:

All tags in the address book are displayed with the same color.

It is hard to visually look for contacts sharing the same tags.

Let's add different colors for different tags to make the
differentiation of tags clearer.

Note: For demo purpose, don't merge this to master!

Zhiyuan-Amos · 2018-01-19T09:30:58Z

is this done alrd? :P

yamgent · 2018-01-19T10:47:47Z

is this done alrd? :P

Yeah I will submit a review now :P

CanIHasReview-bot · 2018-01-19T10:48:13Z

v1

@yamgent submitted v1 for review.

[v1 1/1] PersonCard: Add colors to tag labels

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/798/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

eugenepeh · 2018-01-20T05:58:52Z

btw for these don't merge this to master! PRs, should we just create and assign the corresponding Label to them? would be easier to see :P

Zhiyuan-Amos · 2018-01-20T08:27:26Z

should we just create and assign the corresponding Label to them? would be easier to see :P

Yup, thanks for the reminder Eugene ^^

Zhiyuan-Amos · 2018-01-22T01:41:14Z

src/main/java/seedu/address/ui/PersonCard.java

+    }
+
+    /**
+     * Creates the tag labels for {@code person} and attach them to this {@code PersonCard}.


Not sure whether it's necessary to have and attach them to this {@code PersonCard} since you have already mentioned the word tag labels, so it sounds reasonable that it's gonna be on this PersonCard

Zhiyuan-Amos · 2018-01-22T01:49:46Z

src/test/java/seedu/address/ui/testutil/GuiTestAssert.java

@@ -36,6 +38,9 @@ public static void assertCardDisplaysPerson(Person expectedPerson, PersonCardHan
        assertEquals(expectedPerson.getAddress().value, actualCard.getAddress());
        assertEquals(expectedPerson.getTags().stream().map(tag -> tag.tagName).collect(Collectors.toList()),
                actualCard.getTags());
+        expectedPerson.getTags().stream().map(tag -> tag.tagName).forEach(tag ->


this mapping is unnecessary if we store the result of .collect(Collectors.toList()) on line 39.

Zhiyuan-Amos · 2018-01-22T01:50:36Z

src/test/java/seedu/address/ui/testutil/GuiTestAssert.java

@@ -36,6 +38,9 @@ public static void assertCardDisplaysPerson(Person expectedPerson, PersonCardHan
        assertEquals(expectedPerson.getAddress().value, actualCard.getAddress());
        assertEquals(expectedPerson.getTags().stream().map(tag -> tag.tagName).collect(Collectors.toList()),
                actualCard.getTags());
+        expectedPerson.getTags().stream().map(tag -> tag.tagName).forEach(tag ->
+            assertEquals(Arrays.asList("label", PersonCard.getTagColorStyleFor(tag)),


We should extract "label" as a constant (it's just a default style class for all Label objects right?), otherwise it's unintuitive :P

Zhiyuan-Amos · 2018-01-22T01:52:32Z

src/test/java/guitests/guihandles/PersonCardHandle.java

+                .filter(label -> label.getText().equals(tag))
+                .map(Label::getStyleClass)
+                .findFirst()
+                .orElse(null);


we should throw exception instead to make the test fail faster :)

What should I throw here? We don't have a NoSuchTagException class or something.

IllegalArgumentException should do. See here.

CanIHasReview-bot · 2018-01-22T05:14:24Z

v2

@yamgent submitted v2 for review.

[v2 1/1] PersonCard: Add colors to tag labels

(📚 Archive) (📈 Interdiff between v1 and v2)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/798/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

Zhiyuan-Amos

Looks good!

Zhiyuan-Amos · 2018-01-22T07:27:00Z

src/test/java/seedu/address/ui/testutil/GuiTestAssert.java

-                actualCard.getTags());
+
+        List<String> expectedTags = expectedPerson.getTags().stream().map(tag -> tag.tagName)
+                .collect(Collectors.toList());


perhaps we can put a newline before .map(...)

damithc

Cosmetic suggestions

src/main/java/seedu/address/ui/PersonCard.java

damithc · 2018-01-22T15:01:48Z

src/main/java/seedu/address/ui/PersonCard.java

+     * Returns the color style for {@code tagName}'s label.
+     */
+    public static String getTagColorStyleFor(String tagName) {
+        return TAG_COLOR_STYLES[Math.abs(tagName.hashCode()) % TAG_COLOR_STYLES.length];


Give explanatory comments about this calculation?

damithc · 2018-01-22T15:06:20Z

src/test/java/seedu/address/ui/testutil/GuiTestAssert.java

+        assertEquals(expectedTags, actualCard.getTags());
+        expectedTags.forEach(tag ->
+            assertEquals(Arrays.asList(LABEL_DEFAULT_STYLE, PersonCard.getTagColorStyleFor(tag)),
+                    actualCard.getTagStyleClasses(tag)));


A bit of head scratching needed to figure out what is being verified here. use explanatory variables or comments to make it clearer?

Yup, or perhaps we can extract lines 44-47 into a method assertTagsEqual.

Also, we need to update assertCardEquals(PersonCardHandle, PersonCardHandle) right?
Also, I realised that for the expected result Arrays.asList(LABEL_DEFAULT_STYLE, PersonCard.getTagColorStyleFor(tag)), perhaps we shouldn't call PersonCard#getTagColorStyleFor(Tag) cos the actual tags are initialised using that method too. So it's like in some sense, "ownself test ownself".

Zhiyuan-Amos · 2018-01-22T23:36:06Z

src/main/java/seedu/address/ui/PersonCard.java

+    /**
+     * Returns the color style for {@code tagName}'s label.
+     */
+    public static String getTagColorStyleFor(String tagName) {


it seems like this method is public static because it's required by the tests? ><

Zhiyuan-Amos · 2018-01-22T23:39:13Z

src/test/java/seedu/address/ui/testutil/GuiTestAssert.java

+        assertEquals(expectedTags, actualCard.getTags());
+        expectedTags.forEach(tag ->
+            assertEquals(Arrays.asList(LABEL_DEFAULT_STYLE, PersonCard.getTagColorStyleFor(tag)),
+                    actualCard.getTagStyleClasses(tag)));


Yup, or perhaps we can extract lines 44-47 into a method assertTagsEqual.

Also, we need to update assertCardEquals(PersonCardHandle, PersonCardHandle) right?
Also, I realised that for the expected result Arrays.asList(LABEL_DEFAULT_STYLE, PersonCard.getTagColorStyleFor(tag)), perhaps we shouldn't call PersonCard#getTagColorStyleFor(Tag) cos the actual tags are initialised using that method too. So it's like in some sense, "ownself test ownself".

CanIHasReview-bot · 2018-01-27T16:33:50Z

v3

@yamgent submitted v3 for review.

[v3 1/1] PersonCard: Add colors to tag labels

(📚 Archive) (📈 Interdiff between v2 and v3)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/798/3/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

Zhiyuan-Amos · 2018-01-27T23:23:11Z

src/main/java/seedu/address/ui/PersonCard.java

+    /**
+     * Returns the color style for {@code tagName}'s label.
+     */
+    private static String getTagColorStyleFor(String tagName) {


there isn't a need for static here I think

Zhiyuan-Amos · 2018-01-28T00:36:41Z

src/test/java/seedu/address/ui/testutil/GuiTestAssert.java

+    /**
+     * Returns the color style for {@code tagName}'s label.
+     */
+    private static String getTagColorStyleFor(String tagName) {


hmm I'm not too sure about this way of testing though. If we only have a few set values for tagName, then maybe we should hardcode:

if (tagName.equals("foo")) { return "teal"; } else...

Not too sure about it too, I thought it looks kinda weird cos we are calling the exact same code as the main code base, maybe @damithc can comment on this?

@yamgent what do you think of hardcoding the expected values here? As mentioned, it is a bit odd to use the same code to test the a piece of code.

Sure, I can hardcore the expected values. They can seem pretty arbitrary though, but I can add a comment to explain how the values were derived.

damithc

Comment added.

damithc · 2018-01-28T03:53:07Z

src/test/java/seedu/address/ui/testutil/GuiTestAssert.java

+    /**
+     * Returns the color style for {@code tagName}'s label.
+     */
+    private static String getTagColorStyleFor(String tagName) {


@yamgent what do you think of hardcoding the expected values here? As mentioned, it is a bit odd to use the same code to test the a piece of code.

CanIHasReview-bot · 2018-08-09T05:20:15Z

v6

@yamgent submitted v6 for review.

[v6 1/1] PersonCard: Add colors to tag labels

(📚 Archive) (📈 Interdiff between v5 and v6) (📈 Range-Diff between v5 and v6)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/798/6/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

yamgent · 2018-08-09T06:55:56Z

Changelog:

Purely just rebasing, technically should not have any change in logic.

Zhiyuan-Amos · 2018-08-09T08:05:19Z

src/test/java/seedu/address/ui/testutil/GuiTestAssert.java

+            return "grey";
+
+        default:
+            fail(tagName + " does not have a color assigned.");


Oh wait, after #894, we don't use fail anymore.

The tags are all displayed with the same color. It is hard to differentiate between tags visually across contacts. For example, it is hard to visually group all contacts tagged with "colleague". It is also hard to differentiate between those contacts that are tagged with "colleague", and those that are not tagged with "colleague". Let's add different colors to different tag labels. Tags of the same name will always receive the same color, while different tag names should receive different colors if possible. The hashcode of the tag string is used to determine the tag color. This will assign a reasonably random color to each tag, and will also ensure that the tag color remains the same when the user launches the application again in the future (assuming the application version remains the same). The idea of using hashcode is influenced by a developer's suggestion[1]. [1] nus-cs2103-AY1718S1/forum#199

CanIHasReview-bot · 2018-08-18T12:30:59Z

v7

@yamgent submitted v7 for review.

[v7 1/1] PersonCard: Add colors to tag labels

(📚 Archive) (📈 Interdiff between v6 and v7) (📈 Range-Diff between v6 and v7)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/798/7/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

CanIHasReview-bot added the s.ToReview label Jan 19, 2018

eugenepeh approved these changes Jan 20, 2018

View reviewed changes

Zhiyuan-Amos added the pr.Answer Pull request is answer for a tutorial in developer guide. Do not actually merge. label Jan 20, 2018

Zhiyuan-Amos reviewed Jan 22, 2018

View reviewed changes

yamgent added s.Ongoing and removed s.ToReview labels Jan 22, 2018

yamgent force-pushed the getting-started-ui-tag branch from efdbfe4 to db24d16 Compare January 22, 2018 04:08

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Jan 22, 2018

Zhiyuan-Amos approved these changes Jan 22, 2018

View reviewed changes

Zhiyuan-Amos requested a review from damithc January 22, 2018 07:27

damithc suggested changes Jan 22, 2018

View reviewed changes

Zhiyuan-Amos added s.Ongoing and removed s.ToReview labels Jan 22, 2018

Zhiyuan-Amos reviewed Jan 22, 2018

View reviewed changes

Zhiyuan-Amos mentioned this pull request Jan 24, 2018

Rebase PRs used in suggested programming tasks #784

Closed

yamgent force-pushed the getting-started-ui-tag branch from db24d16 to 3df17df Compare January 27, 2018 16:07

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Jan 27, 2018

CanIHasReview-bot requested a review from damithc January 27, 2018 16:33

Zhiyuan-Amos reviewed Jan 28, 2018

View reviewed changes

damithc reviewed Jan 28, 2018

View reviewed changes

yamgent force-pushed the getting-started-ui-tag branch from 3df17df to a0f850b Compare January 28, 2018 08:54

yamgent requested review from damithc and removed request for damithc January 31, 2018 09:15

damithc approved these changes Jan 31, 2018

View reviewed changes

Zhiyuan-Amos removed the s.ToReview label Jan 31, 2018

guekling mentioned this pull request Mar 8, 2018

[#25] Change UI Component: Use different colours for different tags inside task cards CS2103JAN2018-W13-B4/main#24

Merged

yamgent mentioned this pull request Aug 4, 2018

Rebase all answer PRs #891

Closed

7 tasks

yamgent force-pushed the getting-started-ui-tag branch from 167b3d0 to f6542d9 Compare August 9, 2018 03:15

yamgent added the s.Ongoing label Aug 9, 2018

yamgent force-pushed the getting-started-ui-tag branch from f6542d9 to 259e21f Compare August 9, 2018 05:14

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Aug 9, 2018

yamgent requested a review from a team August 9, 2018 05:20

Zhiyuan-Amos approved these changes Aug 9, 2018

View reviewed changes

Zhiyuan-Amos reviewed Aug 9, 2018

View reviewed changes

pyokagan added s.Ongoing and removed s.ToReview labels Aug 16, 2018

yamgent force-pushed the getting-started-ui-tag branch from 259e21f to 1ac2e7c Compare August 18, 2018 12:24

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Aug 18, 2018

yamgent requested a review from Zhiyuan-Amos August 18, 2018 12:31

Zhiyuan-Amos approved these changes Aug 18, 2018

View reviewed changes

yamgent requested a review from damithc August 19, 2018 02:12

damithc approved these changes Aug 19, 2018

View reviewed changes

yamgent removed the s.ToReview label Aug 19, 2018

jkhiani mentioned this pull request Feb 21, 2019

Added colors to tags CS2103-AY1819S2-W16-3/main#8

Merged

damithc closed this Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ui: Add colors to tag labels #798

Ui: Add colors to tag labels #798

yamgent commented Jan 19, 2018 •

edited

Loading

Zhiyuan-Amos commented Jan 19, 2018

yamgent commented Jan 19, 2018

CanIHasReview-bot commented Jan 19, 2018

eugenepeh commented Jan 20, 2018 •

edited

Loading

Zhiyuan-Amos commented Jan 20, 2018

Zhiyuan-Amos Jan 22, 2018

Zhiyuan-Amos Jan 22, 2018

Zhiyuan-Amos Jan 22, 2018

Zhiyuan-Amos Jan 22, 2018

yamgent Jan 22, 2018

Zhiyuan-Amos Jan 22, 2018

CanIHasReview-bot commented Jan 22, 2018

Zhiyuan-Amos left a comment

Zhiyuan-Amos Jan 22, 2018

damithc left a comment

damithc Jan 22, 2018

damithc Jan 22, 2018

Zhiyuan-Amos Jan 22, 2018

Zhiyuan-Amos Jan 22, 2018

Zhiyuan-Amos Jan 22, 2018

CanIHasReview-bot commented Jan 27, 2018

Zhiyuan-Amos Jan 27, 2018

Zhiyuan-Amos Jan 28, 2018

damithc Jan 28, 2018

yamgent Jan 28, 2018

damithc left a comment

damithc Jan 28, 2018

CanIHasReview-bot commented Aug 9, 2018

yamgent commented Aug 9, 2018 •

edited

Loading

Zhiyuan-Amos Aug 9, 2018

CanIHasReview-bot commented Aug 18, 2018

Ui: Add colors to tag labels #798

Ui: Add colors to tag labels #798

Conversation

yamgent commented Jan 19, 2018 • edited Loading

Zhiyuan-Amos commented Jan 19, 2018

yamgent commented Jan 19, 2018

CanIHasReview-bot commented Jan 19, 2018

v1

eugenepeh commented Jan 20, 2018 • edited Loading

Zhiyuan-Amos commented Jan 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Jan 22, 2018

v2

Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Jan 27, 2018

v3

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Aug 9, 2018

v6

yamgent commented Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

CanIHasReview-bot commented Aug 18, 2018

v7

yamgent commented Jan 19, 2018 •

edited

Loading

eugenepeh commented Jan 20, 2018 •

edited

Loading

yamgent commented Aug 9, 2018 •

edited

Loading