Unique pages #61

OliverZiegler · 2018-01-11T23:34:27Z

This is a first implementation for issue #60.

lloy0076

There are various PHP Docs missing, either completely absent or returns missing.

This getTemplaes and getUniquePages could probably just return a collection to avoid the (ugly) use of collect(...) all the time.

And in fact we could even avoid the various functional mapWithKeys etc by adding more semantic names in the trait.

(Yeah, functional programming is great but in the upper level clases it's nicer to see $this->getUniqueSlugs() vs collect($this->getSlugs()->mapWithKey()->foo()->bar())

lloy0076 · 2018-01-13T06:46:44Z

README.md

+`< route_prefix >/unique/< page_function_slug >`
+
+For users to access the editing page for the page `about_us` you could add a menu item like so:
+(remember the url will use the slug of your function)


Colon at end.

lloy0076 · 2018-01-13T06:46:52Z

README.md

@@ -52,6 +52,27 @@ php artisan migrate
 1. Go to **yourapp/admin/page** and see how it works.
 2. Define your own templates in app/PageTemplates.php using the Backpack\CRUD API.

+## Unique pages usage
+
+Unique pages are pages that exist only once. You can not create a second instance nor delete the current one. 


lloy0076 · 2018-01-13T06:48:35Z

src/app/Http/Controllers/Admin/PageCrudController.php

@@ -27,6 +31,8 @@ public function setup($template_name = false)
        $this->crud->setRoute(config('backpack.base.route_prefix').'/page');
        $this->crud->setEntityNameStrings(trans('backpack::pagemanager.page'), trans('backpack::pagemanager.pages'));

+        $template_names = collect($this->getTemplates())->pluck('name');
+        $this->crud->addClause('whereIn', 'template', $template_names);


lloy0076 · 2018-01-13T06:49:23Z

src/app/Http/Controllers/Admin/PageCrudController.php

@@ -27,6 +31,8 @@ public function setup($template_name = false)
        $this->crud->setRoute(config('backpack.base.route_prefix').'/page');
        $this->crud->setEntityNameStrings(trans('backpack::pagemanager.page'), trans('backpack::pagemanager.pages'));

+        $template_names = collect($this->getTemplates())->pluck('name');


I suppose we're stuck with this colllect(...) here.

It would read better as $this->getTemplateNames() though.

lloy0076 · 2018-01-13T06:51:10Z

src/app/Http/Controllers/Admin/UniquePageCrudController.php

+        |--------------------------------------------------------------------------
+        */
+        $this->crud->setModel($modelClass);
+        // don't set route or entity names here. these depend on the page you are editing


Don't set route or entity names here. These depend on the page you are editing.

lloy0076 · 2018-01-13T07:04:32Z

src/app/TraitReflections.php

+    }
+
+    /**
+     * Get all defined templates.


No @return.

lloy0076 · 2018-01-13T07:05:06Z

src/app/UniquePages.php

+    | Unique pages for Backpack\PageManager
+    |--------------------------------------------------------------------------
+    |
+    | Each unique page has its own method, that define what fields should show up using the Backpack\CRUD API.


No comma here!

lloy0076 · 2018-01-13T07:05:25Z

src/app/UniquePages.php

+    |--------------------------------------------------------------------------
+    |
+    | Each unique page has its own method, that define what fields should show up using the Backpack\CRUD API.
+    | Use snake_case for naming and PageManager will generate the page on first edit


Eh? What if it's not snake_case?

This is also a convention I just copied from default page templates behaviour.

lloy0076 · 2018-01-13T07:06:03Z

src/app/UniquePages.php

+    | Use snake_case for naming and PageManager will generate the page on first edit
+    |
+    | Any fields defined here will show up after the standard page fields:
+    | - select template (hidden and fix)


What does "(hidden and fix)" mean?

Improved the comment

lloy0076 · 2018-01-13T07:06:46Z

src/config/pagemanager.php


    // Change this class if you wish to extend the Page model
-    'page_model_class'       => 'Backpack\PageManager\app\Models\Page',
+    'page_model_class'              => 'Backpack\PageManager\app\Models\Page',


If we are going to have this, will it work for the other (non-unique) page class thingos?

I improved the comment. You need to set both config values 'page_model_class' and 'unique_page_model_class' if you want to use the same (overridden) model.

OliverZiegler · 2018-03-07T21:25:47Z

@lloy0076 i fixed your review remarks. Would you review the changes please? 😊

lloy0076 · 2018-03-23T13:31:56Z

src/app/Http/Controllers/Admin/UniquePageCrudController.php

+
+    public function setup()
+    {
+        parent::__construct();


Why is the constructor called from setup?

Good point... this was just a copy&paste from the original controller...

lloy0076 · 2018-03-23T13:32:17Z

src/app/Http/Controllers/Admin/UniquePageCrudController.php

+        $this->crud->setModel($modelClass);
+        // Don't set route or entity names here. These depend on the page you are editing
+
+        // unique pages cannot be created nor deleted


Actually what is the point of this comment?

wanted to inform why we deny this access...
what do you think? remove the comment entirely?

lloy0076 · 2018-03-23T13:34:08Z

src/app/Http/Controllers/Admin/UniquePageCrudController.php

+    }
+
+    /**
+     * Create the page by slug.


This and the function name are slightly out of sync don't we think?

this is why I don't linke comments... function name says it all.. will fix

lloy0076 · 2018-03-23T13:35:34Z

src/app/Http/Controllers/Admin/UniquePageCrudController.php

+    }
+
+    /**
+     * Override trait version to not update the session variable.


I forget why?

we have no "save actions" here and overwrite with save_and_back.
This would geht default for this session in all Crud-Views.
As we don't want to overwrite behaviour when user can't choose, we need this (empty) implementation

added some more comment to make this clear ;)

lloy0076 · 2018-03-23T13:36:17Z

src/app/TraitReflections.php

+     *
+     * As the method name of a unique page will also be the template name in the database,
+     * we must ensure that there are not any equal names defined.
+     * If different Models (and so different tables in the database) are used, this condition must not hold any more.


must not hold vs may not hold? They're different?

OliverZiegler · 2018-04-24T07:02:35Z

Ready for (final?) review :)

OliverZiegler · 2018-06-03T12:12:47Z

@lloy0076 would you please review the current version?

lloy0076

app/ seems to be getting cluttered up with stuff here...is that really the best thing to do?

e.g. app\UniquePages.php (which is a trait?)

OliverZiegler · 2018-06-18T08:03:34Z

@lloy0076 well this is consistent with the previous stuff.. so I think it's currently ok.
I'm working on an all new pagemanager with a more class based approach an especially artisan commands for creating and removing pages. For now I think we should sty with the current implementation.

tabacitu · 2018-10-16T07:39:39Z

Hi @OliverZiegler ,

As I recently said in issue #60 - I totally agree with the feature, but I'm wondering if this is the best way to do it.

(1) The way I see it, since there is only one pages table, there should only be one Pages model - it doesn't really make sense to me to have Pages and UniquePages. This makes it much more difficult for developers to understand how this package works, and I think that's important because this is a package that most people end up changing / customizing / hacking at. Plus, if you put revisions on UniquePages, you probably want them on regular pages too, right? Because they're just one thing - pages. I think the distinction is artificial and maybe we should rethink this a little bit.

(2) Also, I think the two new restrictions that are imposed to pages would be better of as separate restrictions. For example, in my designs the home page looks A LOT like a landing page. So I usually let clients create new pages, that look just like the homepage, as landing pages. This is particularly useful to target different audiences, and point AdWords campaigns towards different landing pages, instead of homepage. They can then tailor those landing pages to that audience's specific needs => better conversion.

--

To achieve the above (which I think is the same need you have, and that this PR treats), for me as a developer, it would be easier to understand if I just have some new properties inside the PageTemplates trait, where I define which of the page templates are non-deletable, and which of the page templates can only have one entry, etc. So just have some properties there that impose some restrictions, instead of having a different Model where I define other "type" of page templates, which are actually "types" of pages :-)

So for a PageTemplates.php file that looked like this (no unique or mandatory pages):

<?php

namespace App;

trait PageTemplates
{
    /*
    |--------------------------------------------------------------------------
    | Page Templates for Backpack\PageManager
    |--------------------------------------------------------------------------
    |
    | Each page template has its own method, that define what fields should show up using the Backpack\CRUD API.
    | Use snake_case for naming and PageManager will make sure it looks pretty in the create/update form
    | template dropdown.
    |
    | Any fields defined here will show up after the standard page fields:
    | - select template
    | - page name (only seen by admins)
    | - page title
    | - page slug
    */

    private function services() {} // includes multiple addField statements, etc
    private function about_us() {} // includes multiple addField statements, etc
    private function home_page()  {} // includes multiple addField statements, etc
    private function contact() {} // includes multiple addField statements, etc
    private function landing_page()  {} // includes multiple addField statements, etc
}

The only difference, for the developer, to adopt this new feature could be to add a few new properties in PageTemplates.php:

<?php

namespace App;

trait PageTemplates
{
    /*
    |--------------------------------------------------------------------------
    | Page Templates for Backpack\PageManager
    |--------------------------------------------------------------------------
    |
    | Each page template has its own method, that define what fields should show up using the Backpack\CRUD API.
    | Use snake_case for naming and PageManager will make sure it looks pretty in the create/update form
    | template dropdown.
    |
    | Any fields defined here will show up after the standard page fields:
    | - select template
    | - page name (only seen by admins)
    | - page title
    | - page slug
    */

+   $templatesThatCannotBeAdded = ['contact']; // will not show up as options when creating new pages
+   $templatesThatCannotBeDeleted = ['home_page', 'contact']; // entries using these templates will not have a Delete button
+   $entriesThatCannotBeDeleted = [1, 5]; // these entries will not have a Delete button
+   $entriesWhereTheSlugCannotBeEdited = [1]; // the slug will be readonly

    private function services() {} // includes multiple addField statements, etc
    private function about_us() {} // includes multiple addField statements, etc
    private function home_page()  {} // includes multiple addField statements, etc
    private function contact() {} // includes multiple addField statements, etc
    private function landing_page()  {} // includes multiple addField statements, etc
}

Then all of the restrictions above could be done in the PageCrudController::setup() and maybe PageCrudController::search().

What do you think about this different way of doing things @OliverZiegler ? Better? Worse? Or just different? :-)

Thanks, cheers!

OliverZiegler and others added 2 commits January 12, 2018 00:25

add unique pages

c6b8314

Apply fixes from StyleCI

c694b06

OliverZiegler added the working on it label Jan 11, 2018

OliverZiegler requested a review from tabacitu January 12, 2018 09:00

OliverZiegler and others added 5 commits January 12, 2018 11:35

Add descriptive comments

7c6cdc6

Add revisions functionality and config option

bbe7b26

Update readme

15bbd58

Add sanity check for distict tempaltes and unique pages

4c43501

Apply fixes from StyleCI

d3f4eb5

OliverZiegler added enhancement and removed working on it labels Jan 12, 2018

Move exmple teplate about_us to UniquePages

78faf3a

OliverZiegler mentioned this pull request Jan 12, 2018

Unique predefined pages #60

Open

lloy0076 suggested changes Jan 13, 2018

View reviewed changes

OliverZiegler and others added 9 commits January 20, 2018 13:26

changes requested in code review

97e203c

Apply fixes from StyleCI

ca7ded7

remove stale code

2dd65c1

Move buttons to top

edc5760

deny list access

f9b9908

Apply fixes from StyleCI

6914305

Set plural entity name

d6148e3

Fix update method

48aceeb

Apply fixes from StyleCI

dde9313

lloy0076 reviewed Mar 23, 2018

View reviewed changes

OliverZiegler and others added 5 commits April 24, 2018 08:48

fix error loading unique pages by slug with backpack 3.4

cda31c1

fix 503 if only using unique pages and no templates are defined

2448c13

update comments

c0d6650

fix distinct condition

5c29894

Apply fixes from StyleCI

c4a1e1f

lloy0076 reviewed Jun 17, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unique pages #61

Unique pages #61

OliverZiegler commented Jan 11, 2018

lloy0076 left a comment

lloy0076 Jan 13, 2018

lloy0076 Jan 13, 2018

lloy0076 Jan 13, 2018

lloy0076 Jan 13, 2018

lloy0076 Jan 13, 2018

lloy0076 Jan 13, 2018

lloy0076 Jan 13, 2018

lloy0076 Jan 13, 2018

OliverZiegler Jan 20, 2018

lloy0076 Jan 13, 2018

OliverZiegler Jan 20, 2018

lloy0076 Jan 13, 2018

OliverZiegler Jan 20, 2018

OliverZiegler commented Mar 7, 2018

lloy0076 Mar 23, 2018

OliverZiegler Mar 26, 2018

lloy0076 Mar 23, 2018

lloy0076 Mar 23, 2018

OliverZiegler Mar 26, 2018

lloy0076 Mar 23, 2018

OliverZiegler Mar 26, 2018

lloy0076 Mar 23, 2018

OliverZiegler Mar 26, 2018

OliverZiegler Apr 24, 2018 •

edited

Loading

lloy0076 Mar 23, 2018

OliverZiegler commented Apr 24, 2018

OliverZiegler commented Jun 3, 2018

lloy0076 left a comment

OliverZiegler commented Jun 18, 2018

tabacitu commented Oct 16, 2018

Unique pages #61

Are you sure you want to change the base?

Unique pages #61

Conversation

OliverZiegler commented Jan 11, 2018

lloy0076 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OliverZiegler commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OliverZiegler Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OliverZiegler commented Apr 24, 2018

OliverZiegler commented Jun 3, 2018

lloy0076 left a comment

Choose a reason for hiding this comment

OliverZiegler commented Jun 18, 2018

tabacitu commented Oct 16, 2018

OliverZiegler Apr 24, 2018 •

edited

Loading