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

Add variables for transitions #21345

Merged
merged 2 commits into from
Dec 20, 2016
Merged

Add variables for transitions #21345

merged 2 commits into from
Dec 20, 2016

Conversation

tomlutzenberger
Copy link
Contributor

Revision of #21334

See also #21266

Copy link
Contributor

@Starsam80 Starsam80 left a comment

Choose a reason for hiding this comment

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

You didn't have to close the old PR, git push --force does exist. If you fix the 2 notes I made, LGTM.

First time doing a GitHub review. No idea how this is going to work

@@ -15,7 +15,7 @@
user-select: none;
border: $input-btn-border-width solid transparent;
@include button-size($btn-padding-y, $btn-padding-x, $font-size-base, $btn-border-radius);
@include transition(all .2s ease-in-out);
@include transition($transition-base);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be its own variable: $btn-transition

@@ -16,7 +16,7 @@
background-color: $thumbnail-bg;
border: $thumbnail-border-width solid $thumbnail-border-color;
@include border-radius($thumbnail-border-radius);
@include transition(all .2s ease-in-out);
@include transition($transition-base);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be its own variable: $thumbnail-transition

@guylepage3
Copy link

@tomlutzenberger
I agree with @Starsam80, you shouldn't keep moving this around from issue to issue to PR to PR. It makes it very difficult for others contributing to follow along.

Again as stated way back in issue #21266 & #21334

I feel that the naming convention for these variables should match btn names, reducing complexity.

The reason being is that this could limit the number of variables to a standard set. Bootstrap is not created to offer "all" solutions but a couple. It's just a framework. Not a full huge behemoth framework that does everything. IMO.

@tomlutzenberger
Copy link
Contributor Author

Yes, I have to admit that I haven't thought of the forced push.
I'll fix those 2 things.

@mdo mdo added this to the v4.0.0-alpha.6 milestone Dec 20, 2016
@mdo mdo merged commit e1653ed into twbs:v4-dev Dec 20, 2016
This was referenced Dec 20, 2016
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.

4 participants