Skip to content

Conversation

@Monatyr
Copy link
Collaborator

@Monatyr Monatyr commented May 11, 2022

Description

Checklist

  • Included code to test these changes
  • Updated Jira

@ksiek127 ksiek127 self-requested a review May 12, 2022 08:15

viewModel = loader.getController();
viewModel.setNpcImage(npcImage);
viewModel.setBackgroundImage("file:assets/popup-background.png"); //images do not load from fxml on my computer
Copy link
Collaborator

Choose a reason for hiding this comment

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

might be a little problem with working with this code, maybe it's because of operating system, but it's fine as long as it works on any of our computers, the app doesn't need to be cross-platform

viewModel.setNpcFrameImage("file:assets/npc-frame.png");
this.setFill(Color.TRANSPARENT);

textPages = List.of(text.split("(?<=\\G.{200})")); //split text into 200-letter pages
Copy link
Collaborator

Choose a reason for hiding this comment

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

splitting question into pages is nice, but we'll need a limit for answer's length, cause dividing answers into pages seems like too much for me (though it's just my opinion)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now I've decided not to implement a back and forth dialogue. Thought of it as more of a guide or a quick tip for a player. I guess that's still up for debate.

public void setPreviousButtonOnClick(EventHandler<? super MouseEvent> callback) {
previousButton.setOnMouseClicked(callback);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM, gj

Copy link
Collaborator

@co012 co012 left a comment

Choose a reason for hiding this comment

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

Looks solid

@Monatyr Monatyr marked this pull request as draft May 17, 2022 15:55
@Monatyr Monatyr marked this pull request as ready for review May 17, 2022 16:17
@Monatyr Monatyr requested review from co012, ksiek127 and mhawryluk May 17, 2022 16:22
Copy link
Collaborator

@co012 co012 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

lgtm

@kkafar kkafar merged commit c6b694a into master May 22, 2022
@kkafar kkafar deleted the @monatyr/RPG-111-npc-conversation branch May 22, 2022 16:16
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.

6 participants