-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add view command #114
Add view command #114
Conversation
…ewing with view command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Good job! The Ui looks pretty good
tierLabel.getStyleClass().add(tier + "-tier"); | ||
|
||
// Add the label to the FlowPane | ||
assignedTier.getChildren().add(tierLabel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. the issue now is that for those with no tier, there is now an awkward gap where Tier originally was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add on, I think you can use a conditional to render and add the label only if the tier != "NA"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Terrific work pleng! I really like the new UI changes with view. Although close command has yet to be added, the tests you have added are also very comprehensive!
*/ | ||
private void ensureSelectedPersonIsValid(ListChangeListener.Change<? extends Person> change) { | ||
while (change.next()) { | ||
if (selectedPerson.getValue() == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps you can use getSelectedPerson
method here to improve maintainability.
private void handleViewCommand(String commandText) { | ||
if (!splitPane.getItems().contains(personDetailsPanelPlaceholder)) { | ||
splitPane.getItems().add(personDetailsPanelPlaceholder); | ||
placeholderImage.setVisible(false); | ||
splitPane.setDividerPositions(0.6); | ||
} | ||
|
||
String[] commandParts = commandText.split("\\s+"); | ||
if (commandParts.length > 1) { | ||
try { | ||
int index = Integer.parseInt(commandParts[1]) - 1; // Assuming 1-based indexing in UI | ||
Person personToView = logic.getFilteredPersonList().get(index); | ||
personDetailPanel.setPersonDetails(personToView); | ||
} catch (NumberFormatException | IndexOutOfBoundsException e) { | ||
// Handle invalid index | ||
resultDisplay.setFeedbackToUser("Invalid person index."); | ||
} | ||
} else { | ||
resultDisplay.setFeedbackToUser("Please provide a person index to view."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nicely done!
tierLabel.getStyleClass().add(tier + "-tier"); | ||
|
||
// Add the label to the FlowPane | ||
assignedTier.getChildren().add(tierLabel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add on, I think you can use a conditional to render and add the label only if the tier != "NA"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work - super clean code and looks great!
We should discuss whether a user image is necessary since there is no function to change it at the moment but we can just merge anyways, we can deal with it later.
if (person != null && !filteredPersons.contains(person)) { | ||
throw new PersonNotFoundException(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (person != null && !filteredPersons.contains(person)) { | |
throw new PersonNotFoundException(); | |
} | |
requireNonNull(person); | |
if (!filteredPersons.contains(person)) { | |
throw new PersonNotFoundException(); | |
} |
Could consider doing this to match what the module did, but I have no issues with the current implementation either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing out!
resultDisplay.setFeedbackToUser(commandResult.getFeedbackToUser()); | ||
|
||
if (commandText.trim().toLowerCase().startsWith("view")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth refactoring the predicate condition to a isViewCommand()
method, similar to the isShowHelp()
method.
Co-authored-by: leyew <[email protected]>
Co-authored-by: leyew <[email protected]>
8ca6273
into
AY2425S1-CS2103T-T14-4:master
closes #104
This command allows users to see detailed information about a specific person in the address book using the following syntax:
view <INDEX>