Pav 1

[pvullioud]

todo : finish scopeIsPublished

[flo-sch]

Thanks @pvullioud :)

Here are a few things we can improve before to merge the PR :

A. Field names (Slide model)

Just for consistency, I would prefer the field published to be renamed is_published, as it is a boolean field.

File: updates/table_update_flosch_slideshow_slides.php File: models/slide/fields.yml File: models/slide/columns.yml

    public function up()
    {
        Schema::table('flosch_slideshow_slides', function ($table) {
            $table->boolean('is_published')->default(0);
            $table->dateTime('published_at')->nullable()->default(null);
            $table->dateTime('unpublished_at')->nullable()->default(null);
        });
    }

    public function down()
    {
        Schema::table('flosch_slideshow_slides', function ($table) {
            $table->dropColumn('is_published');
            $table->dropColumn('published_at');
            $table->dropColumn('unpublished_at');
        });
    }

B. The Slide published scope: I've completed the query scope, and renamed it scopePublished, still for consistency

File: models/Slide.php

    /**
     * Restrict query to published slides only
     */
    public function scopePublished($query)
    {
        $now = Carbon::now();

        return $query
            ->where('is_published', true)
            ->where(function ($query) use ($now) {
                $query
                    ->where(function ($query) use ($now) {
                        $query
                            ->where('published_at', '>', $now)
                            ->orWhereNull('published_at')
                        ;
                    })
                    ->where(function ($query) use ($now) {
                        $query
                            ->where('unpublished_at', '<', $now)
                            ->orWhereNull('unpublished_at')
                        ;
                    })
                ;
            })
        ;
    }

C. The Slideshow component query: I've updated it with the provided scope to query the slideshow and slides... I've also updated the properties, because the ID wasn't really working (I didn't try to use the explorer before)

File: components/Slideshow.php

    public function defineProperties()
    {
        return [
            'slideshow' => [
                'title'         => 'flosch.slideshow::lang.components.slideshow.properties.id.title',
                'description'   => 'flosch.slideshow::lang.components.slideshow.properties.id.description',
                'placeholder'   => Lang::get('flosch.slideshow::lang.components.slideshow.properties.id.placeholder'),
                'type'          => 'dropdown'
            ],
            'numberOfSlide' => [
                'title'             => 'flosch.slideshow::lang.components.slideshow.properties.number_of_slide.title',
                'description'       => 'flosch.slideshow::lang.components.slideshow.properties.number_of_slide.description',
                'placeholder'       => Lang::get('flosch.slideshow::lang.components.slideshow.properties.number_of_slide.placeholder'),
                'type'              => 'string',
                'validationPattern' => '^[0-9]+$',
                'default'           => '5',
            ]
        ];
    }

    public function getSlideshowOptions()
    {
        return SlideshowModel::lists('name', 'id');
    }

    use Flosch\Slideshow\Models\Slideshow as SlideshowModel;

    public function onRun()
    {
        $slideshowId = (int) $this->property('slideshow');
        $numberOfSlide = (int) $this->property('numberOfSlide');

        $slideshowQueryBuilder = SlideshowModel::where('id', '=', $slideshowId)
            ->with(['slides' => function ($query) use ($numberOfSlide) {
                $query->published();

                if ($numberOfSlide > 0) {
                    $query->take($numberOfSlide);
                }
            }])
        ;

        $this->slideshow = $slideshowQueryBuilder->firstOrFail();
    }

Could you please either make the changes, or give me a temporary write access on your fork, so I can push it myself on your PR?

Thanks again for your contribution :)

[pvullioud]

Hi, this sound good. Maybe you can add me to this project as contributor. So I can create the branch directly in this repo