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.

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

[Feature] Show view #226

Closed
welcoMattic opened this issue Nov 7, 2016 · 21 comments
Closed

[Feature] Show view #226

welcoMattic opened this issue Nov 7, 2016 · 21 comments

Comments

@welcoMattic
Copy link
Contributor

At the moment, in the show view file, there's only a dump() of the Model.

May we consider to improve this to display user-friendly data of Models?

I'm ok to start it! ;)

@tabacitu
Copy link
Member

tabacitu commented Nov 7, 2016

Hi @welcoMattic ,

Yes, it was planned all along, it's just been postponed because of more pressing features. I'm very happy to help you out if you want to get started before I find the time. There are a few issues that need to be addressed. Here are my thoughts:

1. How it would work

  • we should find a name for a feature and use it across the board; since resource controllers use "show" in the URL and that's what the method is called, that's probably the most intuitive name for the feature; I did, however, like "Preview" or "View" better, but... hey ho;
  • we already have a number of ways to display db information, in the form of CRUD column types; we could reuse the column types in the Show feature, to allow people to make a pretty Show screen; we'd just need to strip the <td> when loading those views;
  • a semantics problem here, though, is weather it makes sense to load "column types" inside a "show" functionality, but... I may be splitting hairs here;

2. How it would be used

  • the developers should have the ability to add, remove or replace show columns
  • semantics: maybe we could find a better name for the items than "show columns"?
  • the syntax should be similar, if not identical to the columns; so something like:
$this->crud->addShowColumn(); // add one column, at the end of the stack
$this->crud->addShowColumns(); // add multiple columns, at the end of the stack
$this->crud->removeShowColumn('column_name'); // remove a column from the stack
$this->crud->removeShowColumns(['column_name_1', 'column_name_2']); // remove an array of columns from the stack
$this->crud->setShowColumnDetails('column_name', ['attribute' => 'value']);
$this->crud->setShowColumnsDetails(['column_1', 'column_2'], ['attribute' => 'value']);
  • a nice feature we could have here would be the customization of "wrapperAttributes", like in CRUD Fields; this would allow developers to make the Show panel pretty, by having it split in 2-3 columns, if needed;

3. How it would be coded

  • I don't think that the CrudController::show() method should be separated into a trait; it's a Resourceful Controller feature, after all; or should we move all of them into traits?
  • all methods should be encapsulated into a separate PanelTrait (like Columns, Fields, Buttons, etc); so it would lie in Backpack\CRUD\PanelTraits\Show.php;
  • Backpack\CRUD\PanelTraits\AutoSet::setFromDb() should also autoset the Show Columns, just like it does with Columns and Fields;

4. Possible problems

  • we may find it pretty tricky to fix the naming inconsistencies without causing a breaking change in the package;
    • the blade file is already called show.blade.php so that's good;
    • the button, however, is called "preview"; we could just leave that in the folder if anybody is using it and create a new button, "show", which is loaded by default in Buttons::initButtons(), if the developer has called $this->crud->enableShow();;
  • we have to make sure the Access feature also uses "show" for giving/restricting access to CrudPanel features;

@welcoMattic , what do you think? If you do want to work on this, after we pen down the solution I could create a feature branch for you, so we can collaborate there.

Cheers!

@welcoMattic
Copy link
Contributor Author

@tabacitu Woh such a reply! Indeed you've been working on it, I'm ok to continue this work on a separate feature branch.

For now, in my application, I just patch the feature with some Blade tricks in the show view, only. But it could be nice to make properly in CRUD feature.

Go on!

@OwenMelbz
Copy link
Contributor

@tabacitu @welcoMattic hows this coming along?

@ghost
Copy link

ghost commented Dec 14, 2016

Is there any update on this?

@welcoMattic
Copy link
Contributor Author

Hi @skatika @OwenMelbz @tabacitu!

Last weeks were very full of work, I didn't have much time to work on it. With Xmas period coming I don't know if I could work a lot on this feature. But trust me, in January I'll plan some evenings to work on BackPack, especially on show view ;)

@welcoMattic
Copy link
Contributor Author

@tabacitu @OwenMelbz @skatika PR is open 🎉

@jonphipps
Copy link
Contributor

@welcoMattic @tabacitu @OwenMelbz If you guys don't mind, I'd like to take a crack at implementing this based on the specs detailed in #226 (comment). I have a pressing need and would prefer to implement it in a style that's compatible with the current infrastructure. This would probably not use much from the pending PR.

Basically the approach would be to render a 2-column bootstrap table in the show view using the existing column-type names and views, taking a single row and rendering it vertically -- column one containing the headers and column 2 containing the row data. I would name the elements 'showRow' instead of 'showColumn' and autopopulate using the same selection process as fields rather than restrict show to the column limitations.

If this works for everybody I'd be happy to discuss further, including how you would like me to coordinate the work. I'm probably going to need to implement something in the next few weeks.

@pavoltanuska
Copy link
Contributor

I don't know exactly how this is going along, I just wanted to share my approach I just implemented in one of my projects.

Basic overview of what I did:

  • copied the whole edit.blade.php to my project's show.blade.php (apart from some differences), adjusted a couple of things (mostly strings)
  • added $this->data['fields'] = $this->crud->getUpdateFields($id); to show() method
  • added a little jQuery snippet to the show.blade.php:
jQuery(document).ready(function($) {        
    $('form').find(':input').prop('disabled', true);
});
  • removed the @include('crud::inc.form_save_buttons')

It takes all the fields specified for editing and disables them (it also works for select2 inputs).

I'm not saying it's the most beautiful thing under the Sun, but it's still better than dump() approach and it doesn't look like a breaking change to me.

Possible drawbacks:

  • I don't know if it works with tabs (as I don't use them), but I see no reason why it should
  • I didn't test it for all the fields, just the ones I'm using right now

Maybe this'll help someone, at least until the new approach is implemented.

@welcoMattic
Copy link
Contributor Author

Hi guys,

I'm sorry for months of silence. I've really no time to work on this feature. Actually I'm working on Symfony projects. If someone is interested to take this issue, he/she is welcome ;)

@tabacitu
Copy link
Member

tabacitu commented May 8, 2017

Hi @welcoMattic ,

Thanks a lot for the update! No worries, I'll pick it up in Backpack 3.4.
I imagine it's a pretty frustrating transition from Laravel to Symphony, so good luck with that :-)

Cheers!

@mmickelson
Copy link

In other REST implementations I've seen, to view a resource, you would use something like /tags/1 where 1 is the id of the resource. So,

/tags -> list all tags
/tags/create -> create a new tag
/tags/1 -> show details of tag with id 1
/tags/1/edit -> edit tag with id 1

If I understand correctly, you're saying you would have something like /tags/1/show or /tags/show/1?

@lloy0076
Copy link
Contributor

lloy0076 commented May 12, 2017

Here's how this could turn up iteratively:

  • Copy/paste the create or edit functionality (doesn't matter which) and then filter every input field to be disabled (and remove the submit buttons).

And suddenly you've got an odd looking but workable preview.

Then:

  • Remove the reliance on Laravel's pre-population of forms and make it look prettier.

I'll call this the "bike shedding" step :)

@tabacitu
Copy link
Member

@mmickelson - the route is already there, it's exactly as you suggested, RESTful (/tags/1). I agree with you that a different route makes no sense.

@lloy0076 - I thought about showing everything in fields too, but it definitely won't look good by default :-) Another problem here - you might show some fields on CREATE, others on UPDATE. Which ones would be visible on SHOW? Plus, you'd have things like ID that you might want visible on SHOW, but not on CREATE or UPDATE... It's tricky...

@jonphipps
Copy link
Contributor

@tabacitu I'm still willing to take this on as an assigned task, implemented the way you have specified, as I described in my previous #226 (comment). There shouldn't be any breaking changes.

@lloy0076
Copy link
Contributor

@tabacitu - aye, I agree it wouldn't look particularly pretty; I was more thinking that a stepping stone towards making it look pretty would be an almost copy/paste of the create or update workflow, get that working, and then make it pretty.

@jonphipps - if I'm reading #226 (comment) correctly, then I think we might be saying the same thing in different ways :)

@jonphipps
Copy link
Contributor

@lloy0076 re: "I think we might be saying the same thing in different ways :)" Not exactly. I'm talking about copying the column default definitions and views, intact, and simply defining a 'show' view of a Db row as a single row from the 'list' view, rather than redisplaying the create/update definitions. The column definitions are much more appropriate for showing a single row from the list columns and the current column API is a better fit.

@lloy0076
Copy link
Contributor

lloy0076 commented May 14, 2017

@jonphipps - ah, yes, that would work.

However, consider:

  1. Sometimes I'll not have ALL the fields displayed in the list view (because some of the tables I work with have 20+ more columns);
  2. So, just showing the column definitions in a list won't show all the data.

In addition, if one were to "steal" the code for addFields then one might be able to get the tab feature for free. I'm not sure but I'm fairly certain one could.

Really, where we differ might be:

  1. You're saying "grab the column definitions and then make that a nice preview";
  2. I'm saying "make a new set of definitions (e.g. addShowFields([])) or (addFields([], 'show'))" and then reuse the current addShowField code somehow

That said, I'm sure everyone would appreciate a PR based on what you've suggested.

@jonphipps
Copy link
Contributor

@lloy0076 I'm going by the requirements defined in this set of specs from @tabacitu and reiterated here:

  • developers should have the ability to add, remove or replace show columns
  • the syntax should be similar, if not identical to the columns;
    I would adjust the described syntax to:
$this->crud->addShowRow();

instead of

$this->crud->addShowColumn();
  • we could reuse the column types in the Show feature, to allow people to make a pretty Show screen; we'd just need to strip the when loading those views;
    I would keep the tag and display the data in a 2-column table, with the possible addition of multiple tables in tabs:
<table id="crudTable" class="table table-bordered table-striped display">
  <thead><tr><td>Field</td<td>Value></td></tr></thead>
<tbody>
<tr>
<td>{{ $showRow['label']}}<td>
  {{-- load the view from the application if it exists, otherwise load the one in the package --}}
  @foreach ($crud->showrows as $showRow)
    @if (!isset($showRow['type']))
      @include('crud::columns.text')
    @else
      @if(view()->exists('vendor.backpack.crud.columns.'.$showRow['type']))
        @include('vendor.backpack.crud.columns.'.$showRow['type'])
      @else
        @if(view()->exists('crud::columns.'.$showRow['type']))
          @include('crud::columns.'.$showRow['type'])
        @else
          @include('crud::columns.text')
        @endif
      @endif
    @endif
  @endforeach
</tr>
</tbody>
<tfoot><tr>
  @if ( $crud->buttons->where('stack', 'line')->count() )
    <th>{{ trans('backpack::crud.actions') }}</th>
   @endif
 </tr></tfoot>
</table>
  • all methods should be encapsulated into a separate PanelTrait (like Columns, Fields, Buttons, etc); so it would lie in Backpack\CRUD\PanelTraits\Show.php;
  • Backpack\CRUD\PanelTraits\AutoSet::setFromDb() should also autoset the Show Columns, just like it does with Columns and Fields;
  • leave the "preview" button in the folder and create a new button, "show", which is loaded by default in Buttons::initButtons(), if the developer has called $this->crud->enableShow();;
  • make sure the Access feature also uses "show" for giving/restricting access to CrudPanel features;

Nice to have:

  • customization of "wrapperAttributes", like in CRUD Fields; this would allow developers to make the Show panel pretty, by having it split in 2-3 columns (or tabs), if needed;

re: "That said, I'm sure everyone would appreciate a PR based on what you've suggested."

@tabacitu had suggested: "If you do want to work on this, after we pen down the solution I could create a feature branch for you, so we can collaborate there."

...and I was looking for that sort of explicit 'Yes, make it so' permission to help with it -- a little more formal and structured than just submitting an out-of-the-blue pull request.

@lloy0076
Copy link
Contributor

#226 (comment) all sounds good to me.

@lloy0076
Copy link
Contributor

lloy0076 commented Sep 27, 2017

#991 is a PR for this.

@tabacitu
Copy link
Member

giphy

Thanks to @AbbyJanke this is now ready to use. composer update and just specify $this->crud->allowAccess('show'); in your EntityCrudController.

Cheers!

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

No branches or pull requests

7 participants