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

feat: add assest and kits to exisiting booking #1364

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

feat: add assest and kits to exisiting booking #1364

wants to merge 23 commits into from

Conversation

rajeshj11
Copy link
Contributor

@rajeshj11 rajeshj11 commented Oct 18, 2024 •
edited
Loading

closes #1245

@rajeshj11 rajeshj11 marked this pull request as draft October 18, 2024 19:08
@rajeshj11 rajeshj11 marked this pull request as ready for review October 18, 2024 22:07
@rajeshj11
Copy link
Contributor Author

@DonKoko please review

DonKoko
DonKoko requested changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@DonKoko DonKoko left a comment

Choose a reason for hiding this comment

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

I have made the first review. This still needs a lot of work in terms of code quality.
I did some quick tests and in terms of functionality it works good, but seeing how many things changed also in different places that can affect many other unrelated features, so if we keep the current approach, it will require lots of more testing.

Please check the comments I have made and address them and once its done I can make another review.

app/routes/_layout+/kits.$kitId.tsx Outdated Show resolved Hide resolved
app/components/assets/actions-dropdown.tsx Outdated Show resolved Hide resolved
app/utils/booking-drop-down-actions.tsx Outdated Show resolved Hide resolved
app/utils/booking-drop-down-actions.tsx Outdated Show resolved Hide resolved
app/routes/_layout+/bookings.update-existing.tsx Outdated Show resolved Hide resolved
app/routes/_layout+/bookings.update-existing.tsx Outdated Show resolved Hide resolved
app/routes/_layout+/bookings.update-existing.tsx Outdated Show resolved Hide resolved
app/routes/_layout+/bookings.update-existing.tsx Outdated Show resolved Hide resolved
app/routes/_layout+/bookings.update-existing.tsx Outdated Show resolved Hide resolved
app/routes/_layout+/bookings.update-existing.tsx Outdated Show resolved Hide resolved
DonKoko
DonKoko requested changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@DonKoko DonKoko left a comment

Choose a reason for hiding this comment

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

Already way better but still some changes pending.

app/components/shared/actions-drop-down.tsx Outdated Show resolved Hide resolved
app/modules/asset/service.server.ts Outdated Show resolved Hide resolved
app/modules/booking/service.server.ts Outdated Show resolved Hide resolved
app/modules/booking/service.server.ts Outdated Show resolved Hide resolved
app/modules/kit/service.server.ts Outdated Show resolved Hide resolved
app/routes/_layout+/assets.$assetId.tsx Outdated Show resolved Hide resolved
app/routes/_layout+/bookings.update-existing.tsx Outdated Show resolved Hide resolved
app/routes/api+/model-filters.ts Outdated
organizationId,
OR: [{ id: { in: (selectedValues ?? "").split(",") } }],
};

const genericKeys = ["deletedAt"];
Copy link
Contributor

Choose a reason for hiding this comment

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

I am having trouble understanding this. Can you explain why you are doing it and add some annotations. Then we can maybe see if there is a better way to manage it.

app/components/assets/booking-actions-dropdown.tsx Show resolved Hide resolved
@rajeshj11 rajeshj11 marked this pull request as draft October 24, 2024 14:06
@rajeshj11 rajeshj11 marked this pull request as ready for review October 25, 2024 14:13
DonKoko
DonKoko requested changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@DonKoko DonKoko left a comment

Choose a reason for hiding this comment

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

Files are still not as I like them

app/routes/_layout+/bookings.update-existing.tsx Outdated Show resolved Hide resolved
@rajeshj11 rajeshj11 marked this pull request as draft October 25, 2024 16:03
@DonKoko DonKoko marked this pull request as ready for review October 28, 2024 10:20
@DonKoko DonKoko marked this pull request as draft October 28, 2024 10:20
@DonKoko DonKoko marked this pull request as ready for review October 28, 2024 10:30
@DonKoko DonKoko marked this pull request as draft October 28, 2024 10:31
@rajeshj11 rajeshj11 closed this Oct 29, 2024
@rajeshj11 rajeshj11 reopened this Oct 30, 2024
@rajeshj11 rajeshj11 marked this pull request as ready for review October 30, 2024 20:54
@rajeshj11 rajeshj11 mentioned this pull request Oct 30, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DonKoko DonKoko Awaiting requested review from DonKoko

Requested changes must be addressed to merge this pull request.

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

Successfully merging this pull request may close these issues.

Feature: Add assets to existing booking from asset page
2 participants
@rajeshj11 @DonKoko