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

Categories: Improved Management #3199

Merged
merged 5 commits into from Jun 3, 2015

Conversation

NicolasSiver
Copy link

Changes:

  • Tree view
  • Tree sorting

Summary:

  • Added vendor script for sortable (License MIT)
  • Redesigned Categories presentation
  • Improved overall user experience
  • Render responsibility moved from server to client side (Recursion)
  • Changes: filter filter:admin.categories.get now is coming through socket, not http server, so It doesn't carry request and response objects.

Known bugs:

  • Dragging full tree (not just single element) could produce non-critical JS console error: Uncaught HierarchyRequestError: Failed to execute 'insertBefore' on 'Node': The new child element contains the parent. for onDragOver event. It's just a small destruction to whom like to investigate developer tools ;)

Preview:

screen shot 2015-05-31 at 10 51 10 am

@julianlam
Copy link
Member

Woo hoo! This is exciting, thanks @NicolasSiver! Going to check this out tonight. 😄

@julianlam
Copy link
Member

@psychobunny @NicolasSiver added a vars.less file. Do you approve of this, or should he add these values to bootstrap/variables.less?


function renderListItem(categoryEntity){
var listItem = $(templates.parse(
'<div class="row">' +
Copy link
Member

Choose a reason for hiding this comment

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

This inline HTML should be in a partial. I noticed you actally removed it from categories.tpl because it is parsed on the client-side, but especially in this case, we should keep it in a partial. templates.parse accepts a path as well, so you can just do templates.parse('partials/admin/categoryRow.tpl');, for example.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, It will be much better to have template in external file. I didn't know about such option.
Does template.js use cache for template fetching? Is there any type of pre-fetching of template?
My concern, will it work Ok in iteration? I mean, if this solution produce 20 HTTP requests for list of 20 items... current solution will be better still.

Copy link
Member

Choose a reason for hiding this comment

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

There is caching in the template engines, yes :)

On Tue, 2 Jun 2015 01:11 Nicolas Siver notifications@github.com wrote:

In public/src/admin/manage/categories.js
#3199 (comment):

  •    list.appendTo(container);
    
  •   sortables[parentId] = Sortable.create(list[0], {
    
  •       group: 'cross-categories',
    
  •       animation: 150,
    
  •       handle: '.icon',
    
  •        dataIdAttr: 'data-cid',
    
  •       ghostClass: "placeholder",
    
  •       onAdd: itemDidAdd,
    
  •       onEnd: itemDragDidEnd
    
  •   });
    
  • }
  • function renderListItem(categoryEntity){
  •    var listItem = $(templates.parse(
    
  •        '<div class="row">' +
    

Yes, It will be much better to have template in external file. I didn't
know about such option.
Does template.js use cache for template fetching? Is there any type of
pre-fetching of template?
My concern, will it work Ok in iteration? I mean, if this solution produce
20 HTTP requests for list of 20 items... current solution will be better
still.


Reply to this email directly or view it on GitHub
https://github.com/NodeBB/NodeBB/pull/3199/files#r31493243.

Copy link
Author

Choose a reason for hiding this comment

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

@julianlam are you sure, that It accepts urls?

Uncaught Error: Syntax error, unrecognized expression: admin/partials/categories/category-item.tpl

Copy link
Member

Choose a reason for hiding this comment

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

are you passing in a string?

On Tue, 2 Jun 2015 13:43 Nicolas Siver notifications@github.com wrote:

In public/src/admin/manage/categories.js
#3199 (comment):

  •    list.appendTo(container);
    
  •   sortables[parentId] = Sortable.create(list[0], {
    
  •       group: 'cross-categories',
    
  •       animation: 150,
    
  •       handle: '.icon',
    
  •        dataIdAttr: 'data-cid',
    
  •       ghostClass: "placeholder",
    
  •       onAdd: itemDidAdd,
    
  •       onEnd: itemDragDidEnd
    
  •   });
    
  • }
  • function renderListItem(categoryEntity){
  •    var listItem = $(templates.parse(
    
  •        '<div class="row">' +
    

@julianlam https://github.com/julianlam are you sure, that It accepts
urls?

Uncaught Error: Syntax error, unrecognized expression:
admin/partials/categories/category-item.tpl


Reply to this email directly or view it on GitHub
https://github.com/NodeBB/NodeBB/pull/3199/files#r31549747.

Copy link
Author

Choose a reason for hiding this comment

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

    var listItem = $(templates.parse(
        '/admin/partials/categories/category-item.tpl',
        categoryEntity
    ));

Copy link
Author

Choose a reason for hiding this comment

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

Also what link is correct? Partial template placed under:

src/views/admin/partials/categories/category-item.tpl

For now, for major amount of links, I'm receiving 404 (I'm trying different ones, to find correct path...). Is it really possible to get template from client-side?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. My apologies for the bad linking as I'm afk:

https://github.com/NodeBB/NodeBB/blob/master/public/src/admin/manage/category.js

See line 200

On Tue, 2 Jun 2015 13:59 Nicolas Siver notifications@github.com wrote:

In public/src/admin/manage/categories.js
#3199 (comment):

  •    list.appendTo(container);
    
  •   sortables[parentId] = Sortable.create(list[0], {
    
  •       group: 'cross-categories',
    
  •       animation: 150,
    
  •       handle: '.icon',
    
  •        dataIdAttr: 'data-cid',
    
  •       ghostClass: "placeholder",
    
  •       onAdd: itemDidAdd,
    
  •       onEnd: itemDragDidEnd
    
  •   });
    
  • }
  • function renderListItem(categoryEntity){
  •    var listItem = $(templates.parse(
    
  •        '<div class="row">' +
    

Also what link is correct? Partial template placed under:

src/views/admin/partials/categories/category-item.tpl

For now, for major amount of links, I'm receiving 404. Is it really
possible to get template from client-side?


Reply to this email directly or view it on GitHub
https://github.com/NodeBB/NodeBB/pull/3199/files#r31551352.

@julianlam
Copy link
Member

I noticed "sortable" got added as a library. We already have sortable as introduced via jQuery UI... does that not work?

@NicolasSiver
Copy link
Author

@julianlam variables.less in Bootstrap, is part of Bootstrap, not for 3-rd party editing.
I don't think that jQuery UI Sortable is capable to provide same UX with same amount of efforts (animation, etc.)

@NicolasSiver
Copy link
Author

@julianlam Template has been moved to external file under partials directory.

@NicolasSiver
Copy link
Author

Guys, would be great, if we speed-up process ;)

@julianlam
Copy link
Member

@NicolasSiver :rage2:

@julianlam
Copy link
Member

Just kidding 😄 let me wrap up a couple of things first

@julianlam
Copy link
Member

Sortable lags a ton when there are 700+ categories 😆 ... just saying.

@julianlam
Copy link
Member

Currently rewriting a portion of your code to use templates.js partials asynchronously, stand by 😄

@NicolasSiver
Copy link
Author

Will these partial calls produce HTTP requests?

@julianlam
Copy link
Member

Just the initial load, so... 1.

@julianlam julianlam merged commit a17c65e into NodeBB:master Jun 3, 2015
@julianlam
Copy link
Member

The changes I made: 478850a

@julianlam julianlam self-assigned this Jun 3, 2015
@julianlam julianlam added this to the 1.0.0 milestone Jun 3, 2015
@NicolasSiver NicolasSiver deleted the categories-parent-acp branch June 4, 2015 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants