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.

Sign up for GitHub

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

Jump to bottom

OH2-415 | OH2-316 | Types / Ages CRUD #689

Merged
merged 11 commits into from
Nov 13, 2024
Merged

OH2-415 | OH2-316 | Types / Ages CRUD #689

merged 11 commits into from
Nov 13, 2024

Conversation

SilverD3
Copy link
Contributor

@SilverD3 SilverD3 commented Nov 4, 2024 •
edited by mwithi
Loading

See OH2-316, OH2-415.

Paired with #498

@mwithi mwithi requested a review from SteveGT96 November 4, 2024 15:22
SteveGT96
SteveGT96 suggested changes Nov 5, 2024
View reviewed changes
Copy link
Contributor

@SteveGT96 SteveGT96 left a comment

Choose a reason for hiding this comment

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

It would be nice to add range validation, and make sure that the input ranges are valid before sending request to the backend.

@mwithi
Copy link
Member

mwithi commented Nov 5, 2024

It would be nice to add range validation, and make sure that the input ranges are valid before sending request to the backend.

it is done like that in GUI, @SilverD3 please have a look at it

@SilverD3
Copy link
Contributor Author

SilverD3 commented Nov 6, 2024

@SteveGT96 Done

@SilverD3 SilverD3 requested a review from SteveGT96 November 6, 2024 09:36
@mwithi mwithi mentioned this pull request Nov 6, 2024
Merged
@mwithi mwithi changed the title OH2-316 | Types / Ages CRUD OH2-415 | OH2-316 | Types / Ages CRUD Nov 6, 2024
@mwithi
Copy link
Member

mwithi commented Nov 6, 2024 •
edited
Loading

Testing... could you please adjust this label? "Types of ages" could be ok?

image

SilverD3
SilverD3 commented Nov 6, 2024
View reviewed changes
src/resources/i18n/en.json Outdated Show resolved Hide resolved
@SilverD3
Copy link
Contributor Author

SilverD3 commented Nov 6, 2024

Testing... could you please adjust this label? "Types of ages" could be ok?

Done!

SteveGT96
SteveGT96 suggested changes Nov 8, 2024
View reviewed changes
Copy link
Contributor

@SteveGT96 SteveGT96 left a comment

Choose a reason for hiding this comment

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

We could refactor Age Type form like follows !

src/components/accessories/admin/types/components/agetypes/ageTypesForm/AgeTypesForm.tsx Outdated Show resolved Hide resolved
src/components/accessories/admin/types/components/agetypes/ageTypesForm/AgeTypeForm.tsx Outdated Show resolved Hide resolved
src/components/accessories/admin/types/components/agetypes/ageTypesForm/types.ts Outdated Show resolved Hide resolved
@mwithi
Copy link
Member

mwithi commented Nov 12, 2024

@SteveGT96 are the two converation resolved then?

@SteveGT96
Copy link
Contributor

@SteveGT96 are the two converation resolved then?

Yes, I think we could move on with that.

mwithi
mwithi approved these changes Nov 13, 2024
View reviewed changes
Copy link
Member

@mwithi mwithi left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot!

@mwithi mwithi merged commit 72918b4 into informatici:develop Nov 13, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SteveGT96 SteveGT96 SteveGT96 approved these changes

@mwithi mwithi mwithi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SilverD3 @mwithi @SteveGT96