Skip to content
This repository was archived by the owner on Oct 9, 2020. It is now read-only.

added feature "discount-codes" #2313

Merged
merged 3 commits into from
Mar 18, 2018

Conversation

simarsingh24
Copy link
Member

Mentions #2296
Screenshots for the change:
screenshot_20180204-030739

@ghost ghost added the needs-review label Feb 3, 2018
@ghost ghost assigned simarsingh24 Feb 3, 2018
@simarsingh24
Copy link
Member Author

simarsingh24 commented Feb 3, 2018

@iamareebjamal @srv-twry @mayank8318 @nikit19 Is this a good way to show discount-codes also this is initial steps (the indentation and refactors will be done later, UI will be improved etc )
This is just for review whether I am working in the right direction, the core logic for getting the discount-codes is right?

@fossasia fossasia deleted a comment Feb 3, 2018
@fossasia fossasia deleted a comment Feb 3, 2018
@fossasia fossasia deleted a comment Feb 3, 2018
@fossasia fossasia deleted a comment Feb 3, 2018
@fossasia fossasia deleted a comment Feb 3, 2018
@fossasia fossasia deleted a comment Feb 3, 2018
@simarsingh24
Copy link
Member Author

@iamareebjamal would that be okay if discount codes are shown here, if yes I would proceed further finalizing everything (UI will be changed a little and some more details of discount codes will be added)

@iamareebjamal
Copy link
Member

I think AboutFragment has lost its actual use case, don't add it there. Create a new section

@simarsingh24
Copy link
Member Author

screenshot_20180206-224915
This is how it looks now, created a new section in navigation for discount codes.
@iamareebjamal please tell me out of these which fields of discount codes should be displayed here since the API is mostly returning null or 0 values.

@fossasia fossasia deleted a comment Feb 6, 2018
@fossasia fossasia deleted a comment Feb 6, 2018
@iamareebjamal
Copy link
Member

Show the fields which only have non null values

Copy link
Contributor

@dr0pdb dr0pdb left a comment

Choose a reason for hiding this comment

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

Please make the download lazy i.e. Download should be done after the user enters the fragment.
Also remove the dependency on the event bus pattern.

@simarsingh24
Copy link
Member Author

@srv-twry yeah that i will do also because in order to view dicount codes Authentication is necessary ie user should be logged in, so that check and downloading part will be moved to fragment

@simarsingh24
Copy link
Member Author

screen shots after changes :
screenshot_20180210-113300

@ghost
Copy link

ghost commented Feb 10, 2018

Hi @simarsingh24!

It looks like one or more of your builds have failed.
I've added the relevant info below to save you some time.

@fossasia fossasia deleted a comment Feb 10, 2018
@fossasia fossasia deleted a comment Feb 10, 2018
@fossasia fossasia deleted a comment Feb 10, 2018
@fossasia fossasia deleted a comment Feb 10, 2018
@simarsingh24
Copy link
Member Author

@iamareebjamal please review

Toast.makeText(getContext(), "User need to be logged in!", Toast.LENGTH_SHORT).show();
Intent intent = new Intent(getActivity(), LoginActivity.class);
startActivity(intent);
}
Copy link
Member

Choose a reason for hiding this comment

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

Several formatting related issues, please resolve

Copy link
Member Author

Choose a reason for hiding this comment

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

made some formatting changes, but couldn't find anything in this highlighted area

Copy link
Member Author

Choose a reason for hiding this comment

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

@iamareebjamal could you please confirm?

@fossasia fossasia deleted a comment Feb 14, 2018
@fossasia fossasia deleted a comment Feb 14, 2018
setUpRecyclerView();
Utils.registerIfUrlValid(swipeRefreshLayout, this, this::refresh);
discountFragmentViewModel = ViewModelProviders.of(this).get(DiscountFragmentViewModel.class);
if(AuthUtil.isUserLoggedIn()) {
Copy link
Member

Choose a reason for hiding this comment

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

here

Utils.registerIfUrlValid(swipeRefreshLayout, this, this::refresh);
discountFragmentViewModel = ViewModelProviders.of(this).get(DiscountFragmentViewModel.class);
if(AuthUtil.isUserLoggedIn()) {
if(NetworkUtils.haveNetworkConnection(getContext())) {
Copy link
Member

Choose a reason for hiding this comment

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

here

downloadDiscountCodes();
}
loadData();
}else {
Copy link
Member

Choose a reason for hiding this comment

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

here

@simarsingh24
Copy link
Member Author

made requested changes and squashed commits

@fossasia fossasia deleted a comment Feb 19, 2018
@fossasia fossasia deleted a comment Feb 19, 2018
if (!TextUtils.isEmpty(field.trim())) {
textView.setVisibility(View.VISIBLE);
field = prefix + field;
textView.setText(field);
Copy link
Member

Choose a reason for hiding this comment

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

Directly set prefix + field in TextView

Copy link
Member Author

Choose a reason for hiding this comment

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

done

}


private void setStringField(TextView textView, String field, String prefix) {
Copy link
Member

Choose a reason for hiding this comment

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

Rename the function appropriately

Copy link
Member Author

Choose a reason for hiding this comment

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

changed

@simarsingh24
Copy link
Member Author

squashed commits

@fossasia fossasia deleted a comment Feb 22, 2018
@@ -44,7 +44,7 @@
<dimen name="heading_text_size">20sp</dimen>
<dimen name="text_size_expanded_title">22sp</dimen>
<dimen name="text_size_extra_large">18sp</dimen>
<dimen name="text_size_large">16sp</dimen>
<dimen name="text_size_large">20sp</dimen>
Copy link
Member

Choose a reason for hiding this comment

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

Why is large 20sp and extra large 18 sp?

Copy link
Member Author

Choose a reason for hiding this comment

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

reverting back to 16sp

changed nav icon


indentation changes


indentation changes


indentation changes


refactor changes


minor change
@simarsingh24
Copy link
Member Author

@iamareebjamal please review

ButterKnife.bind(this, itemView);


}
Copy link
Member

Choose a reason for hiding this comment

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

extra lines


public void bindDiscountCode(DiscountCode discountCode) {

String code = Utils.checkStringEmpty(discountCode.getCode());
Copy link
Member

Choose a reason for hiding this comment

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

extra lines

setStringFieldWithPrefix(discountUsedFor, usedFor, "Used For : ");
setStringFieldWithPrefix(this.discountUrl, discountUrl, "Discount URL : ");

}
Copy link
Member

Choose a reason for hiding this comment

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

Extra line

setStringFieldWithPrefix(discountCodeTextView, code, "");
setStringFieldWithPrefix(discountCodeValue, "Value : "+value, "");
setStringFieldWithPrefix(discountMinQuantity, "Minimum Quantity : "+minQuantity, "");
setStringFieldWithPrefix(discountMaxQuantity, "Maximum Quantity :"+maxQuantity, "");
Copy link
Member

Choose a reason for hiding this comment

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

Spacing between +

I'd recommend using template strings

Copy link
Member Author

@simarsingh24 simarsingh24 Mar 18, 2018

Choose a reason for hiding this comment

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

do you mean use of strings.xml
like this :
<string name="MinQuanString">Minimum Quantity : %1$d </string>

Copy link
Member

Choose a reason for hiding this comment

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

Yes

setStringFieldWithPrefix(discountCodeTextView, code, "");
setStringFieldWithPrefix(discountCodeValue, "Value : "+value, "");
setStringFieldWithPrefix(discountMinQuantity, "Minimum Quantity : "+minQuantity, "");
setStringFieldWithPrefix(discountMaxQuantity, "Maximum Quantity :"+maxQuantity, "");
Copy link
Member

Choose a reason for hiding this comment

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

Rather than using "" empty strings, pass null and check for the same

@fossasia fossasia deleted a comment Mar 18, 2018
@simarsingh24
Copy link
Member Author

@iamareebjamal discount codes are not being displayed now! can you point out what went wrong in the last commit ?

@fossasia fossasia deleted a comment Mar 18, 2018
@ghost ghost removed the needs-review label Mar 18, 2018
@iamareebjamal iamareebjamal merged commit d41e69a into fossasia:development Mar 18, 2018
@ghost ghost added the ready-to-ship label Mar 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants