-
Notifications
You must be signed in to change notification settings - Fork 11
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
Enable standard products #2025
Enable standard products #2025
Conversation
…eature/enable-standard-products
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2025 +/- ##
=======================================
Coverage 82.74% 82.74%
=======================================
Files 257 257
Lines 16170 16170
Branches 2490 2490
=======================================
Hits 13380 13380
Misses 2759 2759
Partials 31 31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional review
It does work per se to enable a standard product.
As you said, styling changes should still be applied, e.g. making the gender/category selectboxes unclickable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code reviewed it again
.optional() | ||
.transform((value) => (value === "" ? undefined : value)), | ||
inShop: z.boolean().optional(), | ||
price: z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HaGuesto JS allows a max value of 2^53 for integers but the graphql spec only allows up to 2^31-1: https://spec.graphql.org/October2021/#sec-Int; adding a lower-than validation here would help to avoid throwing an error in the BE
break; | ||
case "InvalidPriceError": | ||
triggerError({ | ||
message: "Price must be a positive integer number.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically non-negative 😉
import { selectedBaseAtom } from "stores/globalPreferenceStore"; | ||
import { z } from "zod"; | ||
|
||
const SingleSelectOptionSchema = z.object({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing exists in BoxEdit.tsx, maybe combine at some point
front/src/views/EnableStandardProduct/components/EnableStandardProductForm.tsx
Show resolved
Hide resolved
isReadOnly | ||
isDisabled | ||
_disabled={{ color: "black" }} | ||
placeholder="Please select a standard product." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Placeholders are not correct but are never shown either.
@@ -44,6 +45,10 @@ export const STANDARD_PRODUCT_BASIC_FIELDS_FRAGMENT = graphql(` | |||
id | |||
name | |||
} | |||
sizeRange { | |||
id | |||
label |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI the SizeRange type also has a name
field in the API serving as alias for label
@pylipp functionally almost everything is in there. So, you should be able to test already.
I will still work on it, but the rest should mostly be styling.