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

-webkit-print-color-adjust #1001

Closed
Malvoz opened this Issue Mar 12, 2018 · 36 comments

Comments

Projects
None yet
5 participants
@Malvoz

Malvoz commented Mar 12, 2018

Consider adding -webkit-print-color-adjust (MDN Reference) as prefix for color-adjust (CSS Color Module Level 4 Draft)

Both properties keyword values are: economy | exact.

There is a data support suggestion on caniuse.

@ai ai added the support label Mar 12, 2018

@ai

This comment has been minimized.

Member

ai commented Mar 12, 2018

@Malvoz good idea. Do you want to send PR?

@Malvoz Malvoz changed the title from -webkit-color-adjust to -webkit-print-color-adjust Mar 12, 2018

@Malvoz

This comment has been minimized.

Malvoz commented Mar 12, 2018

@ai Although I've been around on Github for a while I'm still not sure how to go about PR's.. What are the implications of a PR?

Edit:
I'll fork this repo and add to /data/prefixes.js?

@ai

This comment has been minimized.

Member

ai commented Mar 12, 2018

@Malvoz oops, sorry :).

PR = pull request.

  1. Click Fork button in Autoprefixer repository page.
  2. It will create your own Autoprefixer’s fork.
  3. Clone it to your computer by git.
  4. All yarn install or npm install.
  5. Change data/prefixes.js.
  6. Create git branch git checkout -b print-color-adjust
  7. Make a commit to the branch.
  8. Push to your fork in GitHub
  9. Open your fork on GitHub, you will see button to send pull request.
@Malvoz

This comment has been minimized.

Malvoz commented Mar 12, 2018

@ai
Thanks, I appreciate the instructions, I guess it's time to stop looking the other away and learn git.. I might find time for it tonight, if this PR can wait? 😆

@ai

This comment has been minimized.

Member

ai commented Mar 12, 2018

Sure. Anyways I will have time for it only in Friday.

@ai ai added the caniuse label Mar 18, 2018

@Malvoz

This comment has been minimized.

Malvoz commented Mar 19, 2018

@ai

I've been busy and I wont be able to priorities learning git within the next couple of days.

I understand this probably shouldn't wait for too long (although probably not a wide usage of this property yet) and because I cannot give you a reliable time frame - if you or anyone else want to PR this then by all means go ahead.

I'll make sure to get on with git until the next contribution I make!

@ai

This comment has been minimized.

Member

ai commented Mar 19, 2018

@Malvoz don’t worry, I will send developer from Cult of Martians. I plan they will close this issue in this week.

@soul-wish

This comment has been minimized.

Contributor

soul-wish commented Mar 19, 2018

@Malvoz @ai don't you mind if I'll try to work on this issue?

@ai

This comment has been minimized.

Member

ai commented Mar 19, 2018

@soul-wish sure!

@soul-wish

This comment has been minimized.

Contributor

soul-wish commented Mar 19, 2018

@ai great! Currently working on a PR to caniuse database.

@YozhikM

This comment has been minimized.

Contributor

YozhikM commented Mar 19, 2018

@ai Ready hack and tests, we are waiting for the approval of the PR

@ai

This comment has been minimized.

Member

ai commented Mar 19, 2018

@YozhikM since @soul-wish made so great Can I Use PR he will have priority. But I will try to merge both PR to keep 2 names in GitHub history.

Just send PRs (fallen tests is OK).

@YozhikM

This comment has been minimized.

Contributor

YozhikM commented Mar 19, 2018

@ai did it

@yuriyalekseyev

This comment has been minimized.

Contributor

yuriyalekseyev commented Mar 20, 2018

Hi!
I found some minor problems in @soul-wish PR to Can I Use and fixed them.

Should I continue to work on this issue and create a PR to Autoprefixer or is it too late?

@ai

This comment has been minimized.

Member

ai commented Mar 20, 2018

@yuriyalekseyev addition PR is good. Hope Can I Use author will find a way how to merge them.

@soul-wish

This comment has been minimized.

Contributor

soul-wish commented Mar 20, 2018

@yuriyalekseyev @ai unfortunately it is not additional PR, it is completely separate PR. So now we have two separate PR's to caniuse db for adding the same feature. Not sure if it is the correct way to do OS. Lets wait for caniuse author reply.

@ai

This comment has been minimized.

Member

ai commented Mar 20, 2018

@soul-wish

This comment has been minimized.

Contributor

soul-wish commented Mar 20, 2018

Don't get me wrong, guys. But instead of trying to do the best possible thing together I see some type of race here. This is not very motivational for any person, especially for OS newcomers. Also, there is a real issue for repo maintainers, because they should check all those issues and PR's, waste much more time and made a decisions at the end of the day.

@yuriyalekseyev

This comment has been minimized.

Contributor

yuriyalekseyev commented Mar 20, 2018

@soul-wish @ai sorry about that! I'm kinda junior, so mistakes is my friends :)
Is there a way to reconfigure it to be an addition PR, or leave it untouched and let caniuse maintainer manage it?

@soul-wish

This comment has been minimized.

Contributor

soul-wish commented Mar 20, 2018

@yuriyalekseyev ok, got it :). For future reference, there are a few options, I think:

  • you can leave some comments on any public PR on GitHub (you can suggest changes to a specific lines/files etc.)
  • you can create a new additional PR from the version in original PR (you can checkout from the same branch as original PR, made necessary modifications and submit additional PR with some explanations for maintainer)

In our case, I think the best options would be to delete my PR completely from Can I Use repo. I don't want to waste Can I Use db maintainer's time really. @ai what do you think? Is it the best option?

@YozhikM

This comment has been minimized.

Contributor

YozhikM commented Mar 20, 2018

@soul-wish I can cancel my PR here if it's necessary, because 3 PRs already

@soul-wish

This comment has been minimized.

Contributor

soul-wish commented Mar 20, 2018

@YozhikM it is not necessary. I hope @ai will be able to merge them somehow :)
One more time: I'm not offended, or something ;). Just want to point all of us to some issues in OS processes. GitHub made so much convenient features, hints and notifications inside issues and PR's and here we are, having two PR's with the same feature into one repo and two PR's with the same feature into another :)

@yuriyalekseyev

This comment has been minimized.

Contributor

yuriyalekseyev commented Mar 20, 2018

@soul-wish so to create an additional PR, my steps are:

  • fork/clone repo from original PR
  • checkout to original PR branch
  • submit PR to original PR
    Did I get all correct?
@soul-wish

This comment has been minimized.

Contributor

soul-wish commented Mar 20, 2018

@yuriyalekseyev I hope so, yeah. After that there should be only original PR with two commits (from me and from you). Do you have some time to try that right now?

@yuriyalekseyev

This comment has been minimized.

Contributor

yuriyalekseyev commented Mar 20, 2018

@soul-wish yup, ofc. I'm here not for a race or competition, but for learning and getting experience. I'll close my PR here and on Can I Use and create additional PRs in ~15 mins.

@soul-wish

This comment has been minimized.

Contributor

soul-wish commented Mar 20, 2018

@yuriyalekseyev 👍 I've added you as collaborator to my Can I Use fork repo (you should receive a notification about that). Now you can clone this repo, make your modification, commit and then push your changes. After that original PR will be automatically updated with your new commit ;)

@soul-wish

This comment has been minimized.

Contributor

soul-wish commented Mar 20, 2018

Great work, @yuriyalekseyev! Our PR to Can I Use repo was updated. Looking forward to maintainer's answer ;)

@soul-wish

This comment has been minimized.

Contributor

soul-wish commented Mar 21, 2018

@yuriyalekseyev our PR to Can I Use repo was merged 🎉

@yuriyalekseyev

This comment has been minimized.

Contributor

yuriyalekseyev commented Mar 21, 2018

@soul-wish great! 🎉

@ai

This comment has been minimized.

Member

ai commented Mar 21, 2018

OK, now it is hard time for me to merge PRs 😅

@ai

This comment has been minimized.

Member

ai commented Mar 21, 2018

Both PRs from @YozhikM and @soul-wish were merged to Autoprefixer. @YozhikM, @soul-wish, and @yuriyalekseyev will be mentioned in ChangeLog. @soul-wish will be the winner of Cult of Martian task.

Is it fare for everyone?

@yuriyalekseyev

This comment has been minimized.

Contributor

yuriyalekseyev commented Mar 21, 2018

@ai sadly, that my commits to @soul-wish PR not shown in repo, but mention in ChangeLog also makes me happy. Thanks!

@ai

This comment has been minimized.

Member

ai commented Mar 21, 2018

@yuriyalekseyev oops. It was the side effect of “squash and merge” :(. Since you did really good fixes in @soul-wish PR I added extra commit with your name. Now your name is in Autoprefixer history too.

@yuriyalekseyev

This comment has been minimized.

Contributor

yuriyalekseyev commented Mar 21, 2018

@ai wow, that is really cool. Thank you guys for a warm welcome 👍

@soul-wish

This comment has been minimized.

Contributor

soul-wish commented Mar 21, 2018

Thanks, guys! Thanks @ai for all your help and patience. And big thanks to @yuriyalekseyev for his fixes and corrections to my original PR's! Great work, everyone. 👍😎

@ai

This comment has been minimized.

Member

ai commented Mar 21, 2018

Released in 8.2

@ai ai closed this Mar 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment