Skip to content

Conversation

@Silabalkan
Copy link
Contributor

-Implemented course creation, editing, and deletion features with confirmation

  • Added a course listing view with filtering and sorting options
  • Created a detailed course page displaying course info, ratings, lessons

- Created the  model for managing course data.
- Implemented the  to handle CRUD operations for courses.
- Added migration file to create  table in the database.
- Updated  routes to include routes for managing courses.
- Created  to seed initial course data into the database.
- Set up view components for displaying course listings and details
…grid layout, styled course cards, and improved button styles with hover effects
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 list - now I had to manually write url to the page
  3. Add validation in create/edit form - for example, when user go to create course page and fills nothing, just click "Save" nothing happens - he should see validation messages under each field like "Title is required"; also we should validate length of fields. Also course title should be unique maybe?
  4. Missing empty state; also filters don't look good - arrow is on the text, but it should be next to text

image

  1. Where is navigation? All pages should have navigation like here.

image

  1. On course index page I see that we have filter "Skill" and we have some options

image

But on create course page I can write anything in skill field - please make some select with skill levels

  1. Please be consistent with design - create course and update course looks different

Create course page

image

Edit course page

image

  1. Missing title on browser card

image

  1. I'm not fan of this solution - we should make custom modals for deleting stuff

image

  1. Course index - If we have filter for date, we should show it somehow

  2. I believe that we don't need ziggy

  3. Please add screenshots to pr.

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

Copy link
Member

Choose a reason for hiding this comment

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

All files in .idea directory should be ignored (I meas that shouldn't be pushed on branch)


$query = Course::query();


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 unnecessary lines - if you run make fix it should be done automatically

class CoursePolicy
{
/**
* Determine whether the user can view any models.
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 comments here?

<template>
<div class="max-w-4xl mx-auto mt-10">
<h1 class="text-2xl font-bold mb-6 text-center">Create New Course</h1>
<form @submit.prevent="submit" class="bg-white p-6 rounded-lg shadow-md space-y-4">
Copy link
Member

Choose a reason for hiding this comment

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

Please fix all GitHub warnings

</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


<style scoped>
</style> No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

Missing empty line at the end ot the file

Comment on lines +49 to +50
<style scoped>
Copy link
Member

Choose a reason for hiding this comment

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

If we don't have scoped style, we don't need this tag - please remove

@@ -0,0 +1,24 @@
import { createRouter, createWebHistory, RouteRecordRaw } from 'vue-router';

// Sayfa bileşenlerini import et
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this comment - is really necessary?



Route::middleware(["auth"])->group(function (): void {
// course crud route
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 all unnecessary comments

Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't change permissions to these files 👀

…uced button sizes, and centered the 'Apply' button. Increased navbar font size and weight.
…ced button sizes, and centered the 'Apply' button. Increased navbar font size and weight.
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