Skip to content

Conversation

@akmittal006
Copy link

@akmittal006 akmittal006 commented Jan 20, 2017

a UI update which includes following:

  1. change of header color based on type of template selected. Provides more continuity and feel to app.
    ezgif com-5da98cc0ca

  2. Refactors some code

  3. fixes Selecting and unselecting items #364

@opticod opticod force-pushed the bug-fixes branch 3 times, most recently from a3993ce to 6eea6bd Compare January 29, 2017 14:13
Copy link
Collaborator

@opticod opticod left a comment

Choose a reason for hiding this comment

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

@akmittal006, Thanks for these changes, but it looks like @vishwesh3 is also working for issue #364. Can you please remove the third fix from this PR, so that we can merge it.

Note: Squash your commits into one , Thank you

@opticod opticod mentioned this pull request Mar 11, 2017
@akmittal006 akmittal006 force-pushed the bug-fixes branch 2 times, most recently from 2709181 to fc8fad6 Compare March 12, 2017 12:10
@akmittal006
Copy link
Author

@opticod it is done.
Changes made -

  1. Header color change based on type of template
  2. Refactoring of some code.

Copy link
Collaborator

@opticod opticod left a comment

Choose a reason for hiding this comment

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

Nice effort! But couple of comments.

private static final int TYPE_HEADER = 0;
private static final int TYPE_ITEM = 1;
private final Context context;
private static Context context;
Copy link
Collaborator

@opticod opticod Mar 14, 2017

Choose a reason for hiding this comment

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

Using static field for Context variable will lead to memory leak. You can find many article on memory leak through static fields in web.
Can you come up with different approach for this?

private static final int TYPE_HEADER = 0;
private static final int TYPE_ITEM = 1;
private final Context context;
private static Context context;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here!

@akmittal006
Copy link
Author

@opticod I have made the changes. Thanks for reviewing.

Copy link
Collaborator

@opticod opticod left a comment

Choose a reason for hiding this comment

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

Thanks @akmittal006 Its looking as well as working great. 🎉

@opticod opticod merged commit 8ff1222 into BuildmLearn:bug-fixes Mar 14, 2017
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.

2 participants