Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Pagination Directive #795

Closed
wants to merge 1 commit into from

Conversation

trainerbill
Copy link
Contributor

closing #725

changed to master

@trainerbill
Copy link
Contributor Author

@ilanbiala I closed the last PR since it was going against 0.4.0. Can we get this agreed on and merged? @lirantal any comments?

@lirantal
Copy link
Member

@trainerbill - need more Angular eyes to review this: @codydaig @mleanos @rhutchison

<ng-include src="meanPaginationUrl"></ng-include>
</div>
</div>
<input data-ng-show="meanPaginationSearch === 'true'" class="form-control col-md-4" type="text" data-ng-model="search" placeholder="Search" data-ng-change="figureOutItemsToDisplay()" />
Copy link
Member

Choose a reason for hiding this comment

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

no data-

@ilanbiala
Copy link
Member

@lirantal if we are going to move away from Bootstrap, I'd rather not merge this in. That said, we need to decide on that before this to proceed.

@lirantal
Copy link
Member

I don't think anyone's plan is to move away from Bootstrap.
Bootstrap is a fine boilerplate to work with and I never heard anything about replacing it...

@codydaig
Copy link
Member

I'm pulling this down to test and I noticed an issue with the spacing in modules/users/client/views/admin/user-list.client.view.html. It's tabs and it should be spaces.

@lirantal @ilanbiala There was a discussion brought up recently when someone asked how to remove bootstrap, but that's all I remember hearing of that. I personally have no desire to get rid of bootstrap.

@lirantal
Copy link
Member

There's no plan to move out of bootstrap.
@trainerbill can you address the tab issues?

@ilanbiala
Copy link
Member

@lirantal ui-bootstrap hasn't been updated in a long time and Bootstrap's syntax is in no way semantic. I wouldn't add any code that depends on it, because frankly it isn't a framework that is well suited for Angular right now.

@trainerbill
Copy link
Contributor Author

@ilanbiala Looks like ui-boostrap gets continuous updates:

https://github.com/angular-ui/bootstrap/commits/master

unless you are talking about bumping the version in MEAN. What are the alternatives:

  1. Reinventing the wheel and making our own pagination
  2. Copying the ui-bootstrap pagination directive and modifing accordingly.
  3. Submitting PR to ui-boostrap to add a search feature on pagination
  4. Close the PR. I like the directive. It makes it easy to add pagination and removes a lot of the logic from the user.admin controller.

I will fix the indentation and let you guys figure out how to proceed.

@trainerbill trainerbill force-pushed the PaginationDirective2 branch from 2a4541b to f0837a1 Compare August 18, 2015 16:26
Couple cleanups from our last convo

Indent fix
@trainerbill trainerbill force-pushed the PaginationDirective2 branch from 9de5d30 to a0ea427 Compare August 18, 2015 16:53
@trainerbill
Copy link
Contributor Author

Indentation is fixed. Took me like 5 commits to get it to work so I rebased.

@lirantal
Copy link
Member

@ilanbiala ?

@codydaig codydaig mentioned this pull request Aug 19, 2015
@trainerbill
Copy link
Contributor Author

Whats the word on this? I was going to write some tests for the directive but if its not going to get merged I am not going to waste my time. @lirantal @codydaig @ilanbiala

@ilanbiala
Copy link
Member

@trainerbill I'm not fond of building upon Bootstrap right now, I don't think it's the best idea since it hasn't kept up with Angular and we are planning on updating Angular.

@trainerbill
Copy link
Contributor Author

@ilanbiala Fair enough. The user admin module currently depends on it. Maybe we should look into doing a onmousewheel event to keep loading additional values into the list? That combined with the search function may be suitable and I could prolly make a directive for it.

Also are you talking about boostrap-ui? Is there any other parts of the application that are using it? If not we should work to removing it as a dependency.

@simison
Copy link
Member

simison commented Sep 8, 2015

There's a ticket for Material design #369

It's sorta bad timing right now when it comes to Angular + CSS frameworks...

@codydaig
Copy link
Member

codydaig commented Sep 9, 2015

Bootstrap discussion aside, I like the pagination directive. Just write tests for it and make it fit the style guide and I think it should be good

@ilanbiala
Copy link
Member

@codydaig once we move to Angular 1.4 or even 1.5, then ui-bootstrap will be an issue. I'd rather not add more code that prevents us from easily updating Angular, because that is a big priority since we are quite behind.

@codydaig
Copy link
Member

codydaig commented Sep 9, 2015

@ilanbiala Sorry, should have read the chain of messages a little more throughly.

@ilanbiala
Copy link
Member

@codydaig all good, I just want to make sure that anything we do won't inhibit our attempts at updating the core parts of the framework.

@lirantal
Copy link
Member

lirantal commented Sep 9, 2015

@ilanbiala Angular 1.4 or 1.5? the hell with that. Let's just move straight to 2.0! :-)

@lirantal
Copy link
Member

lirantal commented Sep 9, 2015

So I guess we are closing this one?

@ilanbiala
Copy link
Member

@lirantal 2.0 still seems quite far away and honestly it seems like too much of a change for most developers. Since 1.5 is supposed to act as a migration update, I think we should go to 1.5 and work from there. Closing for now. @trainerbill a generic pagination directive may work, though we may need to do a little updating to get it to work with 1.5.

@ilanbiala ilanbiala closed this Sep 9, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants