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

Allow viewing summary for a selected row #44

Merged
merged 1 commit into from
Dec 11, 2016

Conversation

riwu
Copy link
Contributor

@riwu riwu commented Dec 9, 2016

Allows clicking on a row to view the contribution statistics of the author associated with the row.
Pressing the escape key will return to the summary.

@damithc
Copy link
Contributor

damithc commented Dec 10, 2016

@sebastianquek for your review.

@damithc damithc changed the title mouse click shortcut for view and escape key for summary Allow viewing summary for a selected row Dec 10, 2016
Copy link
Contributor

@sebastianquek sebastianquek left a comment

Choose a reason for hiding this comment

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

Overall, looks good! 👍

Do take note of the formatting, I've followed the Java Coding Standards as mentioned in CS2103.

@@ -74,11 +75,12 @@ private void initLogic() {
}

private void addCommandBar(MainApp mainApp) {
rootLayout.setBottom(new CommandBarController(mainApp));
commandBarController = new CommandBarController(mainApp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace formatting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's whitespace formatting? Is it a different problem from tab indentation?

Also do you indent empty lines?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the same as tab indentation, use 4 spaces.
No, do not indent empty lines!

handleEnterPress(commandBarController, userInput);
handleEnterPress(userInput);
} else if (key == KeyCode.ESCAPE) {
addSummary(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use 4 spaces to indent, not tabs

@@ -127,4 +128,11 @@ private void handleEnterPress(CommandBarController commandBarController,
}
commandBarController.clear();
}

public void handleMouseClick(AuthorBean selectedAuthor) {
String authorName = selectedAuthor.nameProperty().get();
Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace formatting, this includes the several lines after this.

@@ -33,7 +33,7 @@

private static final String OVERVIEW_LAYOUT_FXML = "/main/resources/layouts/Summary.fxml";

public SummaryController(Collection<Author> inputSummaryData) {
public SummaryController(MainApp mainApp, Collection<Author> inputSummaryData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use 4 spaces

@@ -50,6 +50,10 @@ public SummaryController(Collection<Author> inputSummaryData) {
}

summaryTable.setItems(summaryData);
summaryTable.getSelectionModel().selectedItemProperty().addListener((observableValue, oldValue, newValue) -> {
mainApp.handleMouseClick(newValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use 4 spaces

@riwu
Copy link
Contributor Author

riwu commented Dec 10, 2016

Fixed the tab indentation with autoformatting, which removed a few pre-existing empty line indentation as well, let me know if I missed anything.
Do you want the commits to be squashed into one btw?

ObservableList<AuthorBean> summaryData = FXCollections.observableArrayList();
for (Author author : inputSummaryData) {
summaryData.add(new AuthorBean(author));
}

summaryTable.setItems(summaryData);
summaryTable.getSelectionModel().selectedItemProperty()
.addListener((observableValue, oldValue, newValue) -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also indent this block to match the previous statement's dot separator?

@sebastianquek
Copy link
Contributor

@riwu Sure, you can squash the commits into one! Thanks!!

@sebastianquek sebastianquek merged commit 2a55863 into se-edu:master Dec 11, 2016
@riwu riwu deleted the clickToView branch December 13, 2016 05:39
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.

3 participants