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

Add comments feature #30

Merged
merged 20 commits into from
Jun 10, 2020
Merged

Add comments feature #30

merged 20 commits into from
Jun 10, 2020

Conversation

tttzach
Copy link
Owner

@tttzach tttzach commented Jun 4, 2020

I have coded a simple comments form into the website, the text typed into the box will be saved into a List structure and then contents in the List is returned right below the comment box.

Screenshot 2020-06-03 at 8 40 51 PM

@tttzach tttzach requested review from antmar and gbuenoandrade June 4, 2020 01:23
@tttzach tttzach mentioned this pull request Jun 4, 2020
@tttzach tttzach mentioned this pull request Jun 4, 2020
@tttzach tttzach requested a review from gbuenoandrade June 4, 2020 23:46
@tttzach
Copy link
Owner Author

tttzach commented Jun 4, 2020

I have merged another walkthrough's PR with this one since this PR is somewhat small. I hope that's okay!

@tttzach
Copy link
Owner Author

tttzach commented Jun 5, 2020

Reminder to self: add final keyword to variables where possible

@tttzach
Copy link
Owner Author

tttzach commented Jun 5, 2020

I have implemented most of the suggestions and created a new tab specifically for the comments feature. That should be all the content for this walkthrough.

Screenshot 2020-06-05 at 7 28 53 PM

@tttzach tttzach requested a review from gbuenoandrade June 5, 2020 23:30
Copy link
Collaborator

@gbuenoandrade gbuenoandrade left a comment

Choose a reason for hiding this comment

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

Try adding this https://github.com/github/gitignore/blob/master/Java.gitignore to your .gitignore so you don't commit Java bytecode and other binaries you ended up adding to this PR.

@tttzach
Copy link
Owner Author

tttzach commented Jun 8, 2020

Try adding this https://github.com/github/gitignore/blob/master/Java.gitignore to your .gitignore so you don't commit Java bytecode and other binaries you ended up adding to this PR.

I see what you mean, sorry I should've caught that! That has been added to my .gitignore but I need to do a git remove cache to make git acknowledge the changes on my .gitignore and stop tracking the folders I've put in it. Thanks!

@tttzach
Copy link
Owner Author

tttzach commented Jun 8, 2020

I just did a lot of refactoring especially breaking up functions because it seemed like I didn't pay enough attention to the single responsibility principle last week. I've also implemented Gui's suggestions, thanks again!

@tttzach tttzach requested a review from gbuenoandrade June 8, 2020 19:40
Copy link
Collaborator

@gbuenoandrade gbuenoandrade left a comment

Choose a reason for hiding this comment

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

This looks great! Just added some "FYI" comments.

LGTM

private void deleteAll(DatastoreService datastore, PreparedQuery results) {
for (Entity entity : results.asIterable()) {
Key entityKey = entity.getKey();
datastore.delete(entityKey);
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI: I mentioned to Austin that there's an overloading of this method that accepts java.lang.Iterable<Key> as parameter, so you could remove all those values with a single call, but that would involve casting results.asIterable() into java.lang.Iterable<Key> (stream().map() is a neat way of doing that), but that might be too much work here and end up being not worth it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'll look into this towards the end of the week as I want to do some refactoring. I've created an issue so I won't forget about this #36 Thanks!

Copy link
Owner Author

@tttzach tttzach Jun 12, 2020

Choose a reason for hiding this comment

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

@gbuenoandrade
Hi, this is what I have so far:

private void deleteAll(DatastoreService datastore, PreparedQuery results) {
    Iterable<Key> toDelete = (results.asIterable()).stream().map(entity -> {
      return entity.getKey();
    }).collect(Collectors.toList());
    datastore.delete(toDelete);
 }

but it seems that .stream() is undefined for the type Iterable<Entity> and I am not familiar with any other alternatives. Do you have anything in mind?

portfolio/src/main/webapp/script.js Show resolved Hide resolved
This was referenced Jun 10, 2020
@tttzach tttzach merged commit 3f1ee95 into master Jun 10, 2020
@antmar
Copy link
Collaborator

antmar commented Jun 12, 2020

Code looks ok. Just some comments about PR scope, to follow up on discussion today.

  • Changes to .gitignore should always go into a separate PR, because they never interact with any feature/code in your project
  • The first commit should definitely have been a separate PR, because the functionality was subsequently removed. f49ecf0
  • Each of these features could have been a separate PR:
    • Add and list comments
    • Limit # of comments displayed
    • Delete comments
    • Expanding grid / tabs

@tttzach
Copy link
Owner Author

tttzach commented Jun 12, 2020

Thanks for the comments @antmar, I'll definitely reduce my PR scope and follow the workflow that you have suggested on #34. The examples you gave are extremely helpful for me to visualize how my future PRs should look!

@tttzach
Copy link
Owner Author

tttzach commented Jun 22, 2020

Assigned @gbuenoandrade just for the follow up on #30 (comment)

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