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

Priority feature #121

Merged
merged 3 commits into from
Oct 25, 2020
Merged

Conversation

Joven-Heng
Copy link

close #85

Implemented Priority Feature with UI

Copy link

@ijavierja ijavierja left a comment

Choose a reason for hiding this comment

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

If the abnormality highlighted in the comment was on purpose, then LGTM

case "undefined":
priorityShape.setBorder(new Border(new BorderStroke(ColorPicker.WHITE_BORDER,
BorderStrokeStyle.SOLID, CornerRadii.EMPTY, new BorderWidths(BORDER_SIZE))));
priorityShape.setBackground(new Background(new BackgroundFill(Color.WHITE, CornerRadii.EMPTY,

Choose a reason for hiding this comment

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

Is there a difference between ColorPicker.WHITE_BORDER and Color.WHITE?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, there is, white border is slightly darker

Copy link

@WeiJie96 WeiJie96 left a comment

Choose a reason for hiding this comment

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

Just a small comment on the javadoc

import javafx.scene.paint.Color;

/**
* Class for custom colors used for AB3, especially for the priorityShape

Choose a reason for hiding this comment

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

Should this be I4I instead of AB3?

Copy link
Author

Choose a reason for hiding this comment

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

wa shag forgot my bad.

@Joven-Heng
Copy link
Author

I would feel theres a certain level of danger to this because there isnt tests written for these yet, but I assume its just the first iteration of it so yeah LGTM.

Copy link

@WeiJie96 WeiJie96 left a comment

Choose a reason for hiding this comment

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

LGTM

@WeiJie96 WeiJie96 merged commit ac874cb into AY2021S1-CS2103-T16-2:master Oct 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Priority Feature
3 participants