Skip to content

set up suppliers and manufactures to only have Bulk fielda and remove…#14

Open
thebascos wants to merge 1 commit into
devfrom
fix/creating-product
Open

set up suppliers and manufactures to only have Bulk fielda and remove…#14
thebascos wants to merge 1 commit into
devfrom
fix/creating-product

Conversation

@thebascos

@thebascos thebascos commented Feb 5, 2023

Copy link
Copy Markdown
Contributor

… the bulk checkbox and column

set up suppliers and manufactures to only have Bulk fielda and remove the bulk checkbox and column

Provide a description on what was changed/added/fixed in this PR

Checklist

First, create a draft pull request. Then mark the PR as ready for review when all necessary checkbox items has been completed

  • Reviewed own code
  • Commented on code that is hard to understand
  • Implemented tests for the feature/bugfix
  • All GitHub status checks pass
  • The frontend and backend PR previews have been deployed

Backend only

  • Generated the client api if there are new or deleted endpoints and/or DTOs

@railway-app

railway-app Bot commented Feb 5, 2023

Copy link
Copy Markdown

This PR was not deployed automatically as @thebascos does not have access to the Railway project.

In order to get automatic PR deploys, please add @thebascos to the project inside the project settings page.

@vercel

vercel Bot commented Feb 5, 2023

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
logistech ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 5, 2023 at 3:19PM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
logistech-staging ⬜️ Ignored (Inspect) Feb 5, 2023 at 3:19PM (UTC)

Comment on lines +33 to +42
const isSuppManu = () => {
if (
companyType === CompanyDTOTypeEnum.Supplier ||
CompanyDTOTypeEnum.Manufacturer
) {
return true;
} else {
return false;
}
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we can simplify this to just be

Suggested change
const isSuppManu = () => {
if (
companyType === CompanyDTOTypeEnum.Supplier ||
CompanyDTOTypeEnum.Manufacturer
) {
return true;
} else {
return false;
}
};
const isSuppManu = () => {
return companyType === CompanyDTOTypeEnum.Supplier || companyType === CompanyDTOTypeEnum.Manufacturer
};

also, we can't just do:

companyType === CompanyDTOTypeEnum.Supplier || CompanyDTOTypeEnum.Manufacturer

like you had it because what that condition is saying is companyType === CompanyDTOTypeEnum.Supplier (which correctly checks if the company type is SUPPLIER) OR CompanyDTOTypeEnum.Manufacturer (which is just checking if this value is defined). instead, we need to do like in my suggestion above:

companyType === CompanyDTOTypeEnum.Supplier || companyType === CompanyDTOTypeEnum.Manufacturer

this way, we check if companyType === CompanyDTOTypeEnum.Supplier (which checks if company type is SUPPLIER) OR companyType === CompanyDTOTypeEnum.Manufacturer (which checks if company type is MANUFACTURER

Comment on lines +41 to +49
const isSuppManu = () => {
if (
companyType === CompanyDTOTypeEnum.Supplier ||
CompanyDTOTypeEnum.Manufacturer
) {
return true;
} else {
return false;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

also, since we reuse this logic, we can extract it to its own function. preferably, if you could extract this to its own custom hook, that would be great :). alternatively, you can also just extract it to a utility function and pass in the companyType as a parameter.

@princhcanal princhcanal force-pushed the dev branch 2 times, most recently from 769b8cf to 6ce7327 Compare May 4, 2023 10:24
@princhcanal princhcanal force-pushed the dev branch 15 times, most recently from 44eb6b6 to 629cf3d Compare May 24, 2023 15:00
@princhcanal princhcanal force-pushed the dev branch 2 times, most recently from 349106c to bc4df5b Compare May 29, 2023 00:11
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