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

[T09-3] Picconso #50

Open
wants to merge 1,538 commits into
base: master
Choose a base branch
from

Conversation

benedictcss
Copy link

Copy link

@eugenepeh eugenepeh 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 brief look through. Great work so far.

Some things to note though,

  • usually user guide should match the current product, so if its not yet implemented, do mark it with a (future feature) or something.
  • You can remove the top navigation bar too as it doesn't fit your product nicely.

Keep up the good work towards v1.1

ifdef::env-github,env-browser[:relfileprefix: docs/]

https://travis-ci.org/se-edu/addressbook-level4[image:https://travis-ci.org/se-edu/addressbook-level4.svg?branch=master[Build Status]]

Choose a reason for hiding this comment

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

instead of removing, you can update them with your own repo link

{empty}[http://www.comp.nus.edu.sg/~damithch[homepage]] [https://github.com/damithc[github]] [<<johndoe#, portfolio>>]
=== Jeffry Lum
image::jeffry.jpg[width="150", align="left"]
{empty}[https://github.com/j-lum[github]] [<<jeffry#, portfolio>>]

Role: Project Advisor

Choose a reason for hiding this comment

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

remember to update the roles too

* has a need to manage a significant number of contacts
* prefer desktop apps over other types
* can type fast
* needs a qiocl amd easy way to edit images

Choose a reason for hiding this comment

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

typo?

Copy link

@eugenepeh eugenepeh left a comment

Choose a reason for hiding this comment

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

Good work, code quality wise seems to be good. Some things, however, need to be updated soon. E.g. restore your Travis CI.

Other things:

  • Landing page

    • url needs to be updated
      image
  • Gh-pages

    • Update/remove to match your product:
      image
  • README

    • Mock-up looks good
  • User-guide

    • looks good
    • features not implemented yet should be clearly marked as Coming in v2.0
  • Developer guide

    • You can remove Appendix A and G if you want
    • {More to be added} can be removed if no longer applicable
    • Use case: Editing an image wheres the app's interaction?
    • Your NFR is a bit closely tied to the features. Perhaps can add a few more generic ones.
    • Still a lot of AddressBook references, which should be refactor or removed.

* Constructor for ChangeImageEvent
*
* @param image Image to replace
* @param target The name of the ImageView to target.

Choose a reason for hiding this comment

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

Can probably name the variables to be more intuitive so that you don't have to further explain?

public final String target;

/**
* Constructor for ChangeImageEvent

Choose a reason for hiding this comment

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

this can be omit as its self explanatory.


private ArrayList<Layer> layers = new ArrayList<>();

public Canvas(Image base){

Choose a reason for hiding this comment

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

missing space before left curly {

//@@author chivent

/**
* Google Client Instance

Choose a reason for hiding this comment

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

need to provide more detail e.g. what is it for.

pangjiahao pushed a commit to pangjiahao/cs2103-u-schedule that referenced this pull request Oct 17, 2018
Update Appveyor and Coveralls badges on Readme
xiaoyeong pushed a commit to xiaoyeong/addressbook-level4 that referenced this pull request Oct 20, 2018
@eugenepeh
Copy link

@chivent @j-lum @ihwk1996 @lancelotwillow @benedictcss

Feedback for v1.2

Please refer to my v1.1 comments for some of the things that you might have missed out.

Photos

  • One of your team members photo was not rendered properly.
    image

Developer Guide

  • Facilitated by UserPrefs but the direct interaction is model?
    image

  • Does not seems to match the method signature, is this intended?
    image

  • Pros and cons not very clearly highlighted for the second part, also do highlight your selected choice and rationale.
    image

  • Typo? 2 of the methods using exact same name. Also, the name for getting the path of execute file is not very intuitive
    image

Overall

  • Introduction in each of your feature can include value that it brings to your product etc.
  • Work on making the product user-testable soon

Bellaaarh pushed a commit to Bellaaarh/addressbook-level4 that referenced this pull request Oct 29, 2018
lancelotzty and others added 17 commits November 8, 2018 02:19
Add a Codacy badge to README.adoc
Add a Codacy badge to README.adoc
# Conflicts:
#	src/main/java/seedu/address/logic/commands/ConvertCommand.java
#	src/main/java/seedu/address/logic/commands/ExampleCommand.java
# Conflicts:
#	src/main/java/seedu/address/logic/commands/ConvertCommand.java
#	src/main/java/seedu/address/logic/commands/ExampleCommand.java
!!!Breaking changes multiple features!!!
!!!Breaking changes multiple features!!!
benedictcss and others added 30 commits November 12, 2018 21:48
Updated the DG to include a testing guide.
…into general-updates

# Conflicts:
#	docs/DeveloperGuide.adoc
Pants wetting exception fixing experience.
Please figure out how to write tests.
Change the percentage in resize to match code.
Reminded users that layer and canvas operations are in fact **NOT** transformations.
Change `layer remove` to `layer delete`.
Added notice that positions will not take anything longer than a int.
Copied tutorial images out so that help will display them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants