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

replace grunt-autoprefixer with gruntpostcss + autoprefixer #18068

Closed
wants to merge 7 commits into from
Closed

replace grunt-autoprefixer with gruntpostcss + autoprefixer #18068

wants to merge 7 commits into from

Conversation

bassjobsen
Copy link
Contributor

No description provided.


var generateCommonJSModule = require('./grunt/bs-commonjs-generator.js');
var configBridge = grunt.file.readJSON('./grunt/configBridge.json', { encoding: 'utf8' });

var browsers = [
'Android 2.3',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use 2 spaces for indents

@bassjobsen
Copy link
Contributor Author

yes thanks, i see. Done.

@XhmikosR
Copy link
Member

Any change in dist files?

@bassjobsen
Copy link
Contributor Author

dist files are unchanged (as expected)

@XhmikosR
Copy link
Member

Then it should be OK.

map: true
map: true,
processors: [
mq4HoverShim.postprocessorFor({ hoverSelectorPrefix: '.bs-true-hover ' }),
Copy link
Collaborator

Choose a reason for hiding this comment

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

needs to be indented another level

@cvrebert cvrebert added this to the v4.0.0-alpha.2 milestone Oct 27, 2015
@cvrebert
Copy link
Collaborator

Modulo the indentation, LGTM.
We'll need to reshrinkwrap after merging this.

@XhmikosR
Copy link
Member

XhmikosR commented Nov 6, 2015

@bassjobsen: can you fetch and rebase and also take care of @cvrebert's comments?

@bassjobsen
Copy link
Contributor Author

i will try it this weekend. I'm not sure if i do understand the rebase thing.

@bassjobsen
Copy link
Contributor Author

Well i tried / try to do the rebase.
i'm not sure i did it right.
I also found some diffs in dist now:

    4893,4894c4893
    <        -o-transition:         transform .3s ease-out, -o-transform .3s ease-out;
    <           transition: -webkit-transform .3s ease-out;
    ---
    >        -o-transition:      -o-transform .3s ease-out;
    4896d4894
    <           transition:         transform .3s ease-out, -webkit-transform .3s ease-out, -o-transform .3s ease-out;
    5289,5290c5287
    <          -o-transition:         transform .6s ease-in-out, -o-transform .6s ease-in-out;
    <             transition: -webkit-transform .6s ease-in-out;
    ---
    >          -o-transition:      -o-transform .6s ease-in-out;
    5292d5288
    <             transition:         transform .6s ease-in-out, -webkit-transform .6s ease-in-out, -o-transform .6s ease-in-out;

@bassjobsen
Copy link
Contributor Author

i do not understand why 31 files are change now? @XhmikosR should i better create a new PR now?

@wolfy1339
Copy link
Contributor

You didn't do the rebase properly, which would account for the extra unwanted commits and file changes.
I would make a new one, it's easier and faster.

@kkirsche
Copy link
Contributor

I'd agree that it's easier and faster to make a new one @bassjobsen. I'd recommend this guide on how to rebase https://help.github.com/articles/about-git-rebase/ if you aren't familiar with it / don't do it often

@XhmikosR
Copy link
Member

git rebase -i upstream/v4-dev

@kkirsche
Copy link
Contributor

@XhmikosR for the sake of being complete, git fetch upstream && git rebase -i upstream/v4-dev

EDIT: I had the wrong syntax for git fetch

@bassjobsen
Copy link
Contributor Author

okay thanks, i will create a new PR soon

@XhmikosR
Copy link
Member

That is not in order to create a new PR...

@bassjobsen
Copy link
Contributor Author

@XhmikosR yes i understand, i will repeat my changes on the latest version and add it to a new PR

@XhmikosR
Copy link
Member

Good catch, indeed it does.

@bassjobsen
Copy link
Contributor Author

@wolfy1339
Copy link
Contributor

That commit won't cut it either. You would need to rewrite the git history of your branch to only have your changes.
When you do a rebase, you need to do a force push by using git push -f, so the git history is replaced and nothing unwanted happens.
Since you don't seem comfortable with rebase, I would recommend that you create a new pull request and not that it superseded this one.

@cvrebert
Copy link
Collaborator

Leave this open. I'm rebasing and merging this myself.

@cvrebert
Copy link
Collaborator

@XhmikosR This is why I always recommend qualifying requests for rebases with "if you are comfortable+familiar with rebasing..."

@cvrebert
Copy link
Collaborator

Rebased. Running Travis build: https://travis-ci.org/twbs/bootstrap/builds/90641415

@cvrebert
Copy link
Collaborator

Build failed. Retrying now that #17652 has been merged.
https://travis-ci.org/twbs/bootstrap/builds/90642551

@cvrebert
Copy link
Collaborator

There seems to be a problem installing phantomjs@1.9.18 under Node.js v4.2.2 & npm@2.14.7.

@cvrebert
Copy link
Collaborator

Forcing npm@3 fixes that problem, but now some unrelated random RVM problem has raised its head:
https://travis-ci.org/twbs/bootstrap/builds/90644598

@cvrebert
Copy link
Collaborator

Filed travis-ci/travis-ci#5092. Travis is missing curl for some reason, which causes the RVM problem.

@XhmikosR
Copy link
Member

Why did you need to upgrade Ruby anyway? I use 2.2 in another project of mine without any issues.

@XhmikosR
Copy link
Member

env:
  matrix:
    - RUBY_VERSION=2.2

before_install:
  - rvm install $RUBY_VERSION
  - rvm use $RUBY_VERSION --fuzzy
  - export GEMDIR=$(rvm gemdir)

@cvrebert: try this, it should work.

@XhmikosR
Copy link
Member

Also, we shouldn't force npm@3 either. I mean, I'm using 2.x locally just fine. Did you try to clear Travis cache just in case?

@XhmikosR
Copy link
Member

@cvrebert
Copy link
Collaborator

Didn't really need to upgrade Ruby. I was just grasping at straws when debugging the RVM issue.

this seems to pass 9b50133
https://travis-ci.org/twbs/bootstrap/builds/90675687

@XhmikosR Looks good, you should open a PR for that.

@cvrebert
Copy link
Collaborator

No, haven't tried clearing the Travis cache.

@XhmikosR
Copy link
Member

Shouldn't I just merge that then?

@cvrebert
Copy link
Collaborator

Nicely done: 7b19dfc

@cvrebert
Copy link
Collaborator

For whatever reason, the RVM error is no longer occurring.

@cvrebert
Copy link
Collaborator

Purged the caches and building one more time to verify whether npm@3 is truly necessary:
https://travis-ci.org/twbs/bootstrap/builds/90687640

@cvrebert
Copy link
Collaborator

Build failed. @XhmikosR Yup, npm@3 is needed to install from an npm@3 shrinkwrap, which is reasonable. I'm guessing you don't see it locally since you're probably not installing via shrinkwrap.

@XhmikosR
Copy link
Member

@cvrebert: we don't use a v3.x shrinkwrap, do we?

@cvrebert
Copy link
Collaborator

@XhmikosR npm v3 is the latest stable major version of npm and seems to the preinstalled version for Node.js v5. And it's compatible with Node.js v4. Since the shrinkwrap is only used internally, I don't see much reason to use an old version.
(My branch switches to npm v3 shrinkwrap since I have Node.js v5 on my local box.)

@cvrebert
Copy link
Collaborator

Rebased and merged as 50c43bc. Thanks @bassjobsen!

@XhmikosR Feel free to argue about 3236a4d tomorrow 😄

@cvrebert cvrebert closed this Nov 13, 2015
@XhmikosR
Copy link
Member

The thing is, I don't want to use v5.0.0 yet... This broke my branch and I will need to use npm 3.x to rebase it.

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

5 participants