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

Create a generic component to update and insert SubCategories #78

Merged
merged 1 commit into from Jul 19, 2020

Conversation

anjula-sack
Copy link
Member

@anjula-sack anjula-sack commented Jul 16, 2020

Purpose

Goals

  • Add editing function to subcategories

Approach

  • Edit AddSubCategory component to edit subcategory
  • Change the HTTP response from 202 ACCEPTED to 200 OK
    Kapture 2020-07-16 at 12 24 04

Checklist

  • This PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
  • I have read and understood the development best practices guidelines ( http://bit.ly/sef-best-practices )
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

Copy link
Member

@jayasanka-sack jayasanka-sack left a comment

Choose a reason for hiding this comment

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

Good job @anjula-sack!
I think we can reuse the form too..

@Gimhan-minion Gimhan-minion added academix Issues or improvements related to Academix project admin-dashboard labels Jul 16, 2020
@anjula-sack anjula-sack force-pushed the edit-subcategory branch 4 times, most recently from 7441520 to 64e7f01 Compare July 17, 2020 06:27
@anjula-sack anjula-sack force-pushed the edit-subcategory branch 2 times, most recently from 429fa9a to 25b8582 Compare July 17, 2020 10:47
Copy link
Member

@jayasanka-sack jayasanka-sack 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 @anjula-sack ,

Consider about the mentioned change requests

@Gravewalker666
Copy link
Member

I think it's better if we could display the SubCategory Id in the edit view, Maybe in a disabled text input or something?
What do you think @jayasanka-sack?

@anjula-sack anjula-sack force-pushed the edit-subcategory branch 3 times, most recently from 53df049 to 60eee6e Compare July 18, 2020 05:37
Copy link
Member

@jayasanka-sack jayasanka-sack left a comment

Choose a reason for hiding this comment

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

Nice to see that the component has improved a lot!

Added some comments, fix them too. :)

this.state = {
isLoading: false,
categories: [],
subcategory: null,
isComponentEdit: false,
Copy link
Member

Choose a reason for hiding this comment

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

instead of saving a boolean. Store the type which accepts only "add" and "edit"

Copy link
Member

Choose a reason for hiding this comment

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

Also, it shouldn't be a state variable since you don't need to rerender the component when the component type changes. (the type isn't changing time to time)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes I will change it.

method = 'post';
url = '/core/academix/admin/sub-categories/';
} else {
statusCode = 202;
Copy link
Member

Choose a reason for hiding this comment

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

This should fix in the backend

Copy link
Member Author

Choose a reason for hiding this comment

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

so what should I do until then?

Copy link
Member

Choose a reason for hiding this comment

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

you can fix the backend with this PR. It's a single line change

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

Comment on lines 185 to 188
{this.state.subcategory != null &&
this.subCategoryForm('Edit Subcategory')}
{this.state.subcategory == null &&
this.subCategoryForm('Add Subcategory')}
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better if you could use a generic string to the button.

What about "Save" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

great.

@jayasanka-sack
Copy link
Member

jayasanka-sack commented Jul 18, 2020

I think it's better if we could display the SubCategory Id in the edit view, Maybe in a disabled text input or something?
What do you think @jayasanka-sack?

Yes, it's good. But according to the use case, the users don't use IDs to identify a subcategory. We use it to programmatically identify it, but a normal user identifies a subcategory by its category's name and the subcategory name.

So I think we don't need to display ids in the UI because it isn't informative data to the user. @YohanAvishke WDYT?

Copy link
Member

@jayasanka-sack jayasanka-sack left a comment

Choose a reason for hiding this comment

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

Nice job @anjula-sack !

Fix the following to improve the component.

@anjula-sack anjula-sack force-pushed the edit-subcategory branch 3 times, most recently from 2bbc9b3 to 0b87449 Compare July 18, 2020 16:24
@anjula-sack anjula-sack force-pushed the edit-subcategory branch 4 times, most recently from 76f866a to d93eac8 Compare July 19, 2020 12:20
@anjula-sack anjula-sack changed the title Reuse AddSubCategory component to edit subcategories Create a generic component to update and insert SubCategories Jul 19, 2020
);
});
};
render() {
Copy link
Member

Choose a reason for hiding this comment

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

Add a space between functions

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

Copy link
Member

@jayasanka-sack jayasanka-sack left a comment

Choose a reason for hiding this comment

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

Thanks @anjula-sack !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
academix Issues or improvements related to Academix project admin-dashboard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a edit component to edit the subcategory
4 participants