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

Pagination / Pager code review and better customizing (second try) #11747

Closed
wants to merge 4 commits into from

Conversation

tlindig
Copy link
Contributor

@tlindig tlindig commented Dec 6, 2013

Reviewed Pagination and Pager.

For Pagination:

  • use mixin ".pagination-size" also for base size
  • make pagination variables naming scheme conform, deprecated old ones
  • make pagination more customizable by supporting variables for color, bg and border in all states (default, hover, active, disabled)
  • harmonize handling for inner 'a' and 'span' tags

For Pager:

  • make pager more customizable by using variables of pagination for color, bg and border in all states (default, hover, active, disabled)
  • deprecated variable '@pager-disabled-color', because it is now a 'duplicate' of a pagination variable '@pagination-disabled-color'
  • harmonize handling for inner 'a' and 'span' tags

This is a replacement of pull request #11684

* make pagination more customizable by supporting variables for color, bg and border in all states (default, hover, active, disabled)
* harmonize handling for inner 'a' and 'span' tags
…olor, bg and border in all states (default, hover, active, disabled)

* deprecated variable '@pager-disabled-color', because it is now a 'duplicate' of a pagination variable '@pagination-disabled-color'
* harmonize handling for inner 'a' and 'span' tags
@zlatanvasovic
Copy link
Contributor

Not bad idea, but can be done much much simpler.

@mdo
Copy link
Member

mdo commented Dec 8, 2013

I don't see an advantage to removing the default sizing from where the rest of the default component settings are being done. Just adds extra CSS.

@tlindig
Copy link
Contributor Author

tlindig commented Dec 8, 2013

@mdo

The advantage would be, that you have one way for defining size and only one place, where it is implemented. If you need a bugfix for that, you could do it at this one place. If someone have other idear of implementing it, he only need to replace the mixin. The rest of code could stay untouched.

@tlindig
Copy link
Contributor Author

tlindig commented Dec 8, 2013

@zdroid

Not bad idea, but can be done much much simpler.

Could you please be more specific?

@zlatanvasovic
Copy link
Contributor

Could you please be more specific?

Too many complications just for customizations. :P

This just enlarges code base, could you try to reduce code?

@tlindig
Copy link
Contributor Author

tlindig commented Dec 9, 2013

Not bad idea, but can be done much much simpler.

Please tell me how.

Drop support for span?

@zlatanvasovic
Copy link
Contributor

No, I'm just talking about your request, not general pagination code.

@tlindig
Copy link
Contributor Author

tlindig commented Dec 9, 2013

I still not know how you will do it "much much simpler". It would be nice, if you could share your solution with us.

@zlatanvasovic
Copy link
Contributor

Avoid that deprecation, avoid default state variables and improve pager
items.

I asked you to reduce code size because of this is like flood.

2013/12/9 Tobias Lindig [email protected]

I still not know how you will do it "much much simpler". It would be nice,
if you could share your solution with us.


Reply to this email directly or view it on GitHubhttps://github.com//pull/11747#issuecomment-30134133
.

Zlatan Vasović - ZDroid

@tlindig
Copy link
Contributor Author

tlindig commented Dec 10, 2013

@zdroid

Avoid that deprecation, avoid default state variables

To follow the propagated name scheme, it looks like it does. For the default state you have default values. I can not see what is wrong with that. The extra (old) variables, that is for backward compatibility, is not shown in the compiled result. So I thing it is OK, how I did it.

and improve pager items.

In which direction?
I improved it a bit. I made it more robust.

I asked you to reduce code size because of this is like flood.

But I can not see how, without loosing the support for customizing Pagination.

BTW: I did this changes for my project and want give it back to the community, because I think it is an improvement. In my project I need a custom pagination.

If you think, you can do it "much much simpler", your are welcome to do it and share it.

From my point of view, this commit is ready. It is as short, clean and robust as I can do it. Take it, change it or leave it.

@mdo
Copy link
Member

mdo commented Dec 15, 2013

Just merged #11840, so this conflicts with the new changes from that. I still don't see a point in the mixin for the base sizing, or the variable renaming since there's no contextual variation for the pagination like buttons or panels.

@mdo mdo closed this Dec 15, 2013
@tlindig
Copy link
Contributor Author

tlindig commented Dec 16, 2013

I still don't see a point in the mixin for the base sizing,

Same code for same result. Want set size, do it in every case in same way.
But this is more a philosophical question. There is no wrong or right.

the variable renaming since there's no contextual variation for the pagination like buttons or panels.

I thought this is the wanted naming scheme.

#6342

More consistent variable naming scheme.

  • The general approach is element, state, pseudo state. For example, @navbar-link-color-hover.

It seams, that I mixed up "state" and "pseudo state". Sorry, my fault.

But the main benefit of this request you did not see. The consequent customizing support for pagination and bug fixing. I will try it a second time and split it in different small commits.

@mdo
Copy link
Member

mdo commented Dec 16, 2013

I will try it a second time and split it in different small commits.

Awesome, I'll keep an eye out for them. Again, always appreciate folks following up in case I missed something, so keep it up.

It seams, that I mixed up "state" and "pseudo state". Sorry, my fault.

Yeah, not a huge deal. My point, to be clear, is the variables added the -default- slug in there, which isn't necessary for the base class as I mentioned earlier.

@zlatanvasovic
Copy link
Contributor

Good so far.

@tlindig
Copy link
Contributor Author

tlindig commented Dec 20, 2013

@mdo One more question about naming scheme for variables.

What is the correct name for a variables that specify a border color?

In variables.less I can found:

@table-border-color
@legend-border-color
@input-group-addon-border-color
@nav-tabs-border-color
@nav-tabs-link-hover-border-color
...

but also

@input-border
@btn-default-border
@dropdown-border
@pagination-border
...

Is there a rule for when to take "border" and when "border-color"?

@zlatanvasovic
Copy link
Contributor

No. It should be -border-color, but it's -border sometimes (will be
fixed in v4).

2013/12/20 Tobias Lindig [email protected]

@mdo https://github.com/mdo One more question about naming scheme for
variables.

What is the correct name for a variables that specify a border color?

In variables.less I can found:

@table-border-color
@legend-border-color
@input-group-addon-border-color
@nav-tabs-border-color
@nav-tabs-link-hover-border-color
...

but also

@input-border
@btn-default-border
@dropdown-border
@pagination-border
...

Is there a rule for when to take "border" and when "border-color"?


Reply to this email directly or view it on GitHubhttps://github.com//pull/11747#issuecomment-31017416
.

Zlatan Vasović - ZDroid

@tlindig
Copy link
Contributor Author

tlindig commented Dec 20, 2013

No. It should be -border-color, but it's -border sometimes (will be fixed in v4).

So if I want add new variables, I should use -border-color, correct?

So if in one component just one border color defined, and I want add new ones for different states, should I rename the old one to prevent name scheme breaks in one and the same component?
Of course I would not remove the old variables, I would set it as deprecated.

Or is it better to use the wrong scheme and wait for v4?

@zlatanvasovic
Copy link
Contributor

-border-color is ok. Ask Mark Otto for more details, I don't know what he
thinks about this (he's Bootstrap CSS "chief").

2013/12/20 Tobias Lindig [email protected]

No. It should be -border-color, but it's -border sometimes (will be fixed
in v4).

So if I want add new variables, I should use -border-color, correct?

So if in one component just one border color defined, and I want add new
ones for different states, should I rename the old one to prevent name
scheme breaks in one and the same component?
Of course I would not remove the old variables, I would set it as
deprecated.

Or is it better to use the wrong scheme and wait for v4?


Reply to this email directly or view it on GitHubhttps://github.com//pull/11747#issuecomment-31019451
.

Zlatan Vasović - ZDroid

@tlindig tlindig deleted the pagination_customizing2 branch January 16, 2014 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants