Skip to content

Conversation

@selinuluca
Copy link

Implemented CRUD functionality for course materials.
Integrated create, read, update, and delete operations into the system.
Connected CRUD features with the Vue.js and Inertia.js front end.

Copy link
Member

@EwelinaSkrzypacz EwelinaSkrzypacz left a comment

Choose a reason for hiding this comment

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

  1. Please fix checks - all are failed.
  2. Add in navigation button to course materials list - now I had to manually write url to the page
  3. I think that we should first merge pr with course and after that we can make change to this code - I mean that for now I can't add course materials because I can't create course now; I see that we have model Lesson but I think we should change this to course
  4. Add validation in create/edit form - for example, when user go to create course material page and fills nothing, just click "Save" I see only frontend validation - please add also backend validation and show message
  5. Missing empty state;

image

  1. Missing title on browser card

image

  1. Please add screenshots to pr.

  2. Please include the task number related to this pull request in the description.

@@ -0,0 +1,258 @@

Copy link
Member

Choose a reason for hiding this comment

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

What is this file?

@@ -0,0 +1,11 @@
= Interns2024c\Models\CourseMaterial {#6442
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this file? Why the name of this file look like line of code to get all materials?

@@ -0,0 +1,12 @@
= Illuminate\Database\Eloquent\Collection {#6273
Copy link
Member

Choose a reason for hiding this comment

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

The same as above

Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?

</div>
</template>

<script>
Copy link
Member

Choose a reason for hiding this comment

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

Please move <script> to the top - like we have for example in Register.vue file

<button
@click="editMaterial(material.id)"
style="
margin-top: 15px;
Copy link
Member

Choose a reason for hiding this comment

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

Please use tailwindcss to style stuff

Route::delete("/profile", [ProfileController::class, "destroy"])->name("profile.destroy");
});

// Debug lessons route
Copy link
Member

@EwelinaSkrzypacz EwelinaSkrzypacz Jan 7, 2025

Choose a reason for hiding this comment

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

Please remove this route - as I understand it's for debugging, so it shouldn't be pushed

Copy link
Member

Choose a reason for hiding this comment

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

Why permissions are changed?

},
setup(props) {
console.log('Lessons prop (raw):', props.lessons);
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this console.log

Added a navigation button to the Course Materials list for easier access.
Previously, the URL had to be manually entered to navigate to the page.
Copy link
Member

@EwelinaSkrzypacz EwelinaSkrzypacz left a comment

Choose a reason for hiding this comment

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

I'll be happy to review this PR once all checks have passed and my feedback has been implemented.

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.

3 participants