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

[author] Update SimpleBar v3.1.x with new auto-update config #13150

Merged
merged 1 commit into from Feb 7, 2019

Conversation

Grsmto
Copy link
Contributor

@Grsmto Grsmto commented Dec 15, 2018

Update SimpleBar after folder structure change.

Thanks!

Pull request for issue: #13102

Git commit checklist

  • The first line of commit message is less then 50 chars; clean, clear and easy to understand.
  • The parent of the commit(s) in the PR is not older than 3 days.
  • Pull request is sent from a non-master branch with a meaningful name.
  • Separate unrelated changes into different commits.
  • Use rebase to squash/fixup dummy/unnecessary commits into only one commit.
  • Close corresponding issue in commit message
  • Mention related issue(s), people in commit message, comment.

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

Oops 😨 f5f7aa6 CI test failed ❗

@Grsmto please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/24781 for details 📝 !
Most of the error will have corresponding explanation, so that you will know what's wrong and then try to fix it!
If you cannot understand the error message and need help, feel free to ask our maintainers ☺️

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

Oops 😨 e05df5d CI test failed ❗

@Grsmto please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/24783 for details 📝 !
Most of the error will have corresponding explanation, so that you will know what's wrong and then try to fix it!
If you cannot understand the error message and need help, feel free to ask our maintainers ☺️

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@Grsmto congratulations! 7a801d1 CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/24806, thank you 😀

Copy link
Contributor

@sashberd sashberd left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

Oops 😨 f988665 CI test failed ❗

@Grsmto please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/25115 for details 📝 !
Most of the error will have corresponding explanation, so that you will know what's wrong and then try to fix it!
If you cannot understand the error message and need help, feel free to ask our maintainers ☺️

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

Oops 😨 835d96f CI test failed ❗

@Grsmto please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/25118 for details 📝 !
Most of the error will have corresponding explanation, so that you will know what's wrong and then try to fix it!
If you cannot understand the error message and need help, feel free to ask our maintainers ☺️

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@Grsmto congratulations! b263e8d CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/25133, thank you 😀

@extend1994
Copy link
Contributor

extend1994 commented Jan 7, 2019

Hi @Grsmto Could you also commit latest version and specify it in package.json with another commit ? When I attempted to do that for you, it shows that ! [remote rejected] simplebar -> refs/pull/13150/head (permission denied).
After that version is added, the other versions would be added by cdnjs bot @PeterBot

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

Oops 😨 764bb1f CI test failed ❗

@Grsmto please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/25161 for details 📝 !
Most of the error will have corresponding explanation, so that you will know what's wrong and then try to fix it!
If you cannot understand the error message and need help, feel free to ask our maintainers ☺️

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

Oops 😨 2307c10 CI test failed ❗

@Grsmto please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/25162 for details 📝 !
Most of the error will have corresponding explanation, so that you will know what's wrong and then try to fix it!
If you cannot understand the error message and need help, feel free to ask our maintainers ☺️

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@Grsmto congratulations! b4d5e91 CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/25163, thank you 😀

Copy link
Contributor

@extend1994 extend1994 left a comment

Choose a reason for hiding this comment

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

Usually, we add the file from npm/GitHub to make sure file integrity, not sure if the differences below are fine?

ajax/libs/simplebar/3.1.1/simplebar.js Outdated Show resolved Hide resolved
ajax/libs/simplebar/3.1.1/simplebar.js Outdated Show resolved Hide resolved
Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

Oops 😨 3a14228 CI test failed ❗

@Grsmto please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/25292 for details 📝 !
Most of the error will have corresponding explanation, so that you will know what's wrong and then try to fix it!
If you cannot understand the error message and need help, feel free to ask our maintainers ☺️

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@Grsmto congratulations! 26072da CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/25293, thank you 😀

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@Grsmto congratulations! 45f6074 CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/25298, thank you 😀

@Grsmto
Copy link
Contributor Author

Grsmto commented Jan 21, 2019

@extend1994 sorry for the ping but we might be good to merge?

@extend1994
Copy link
Contributor

@Grsmto Sorry for the delay, I think *.esm.* should be removed in this PR?
Other files are good to go now 👍

@olafcm
Copy link
Contributor

olafcm commented Feb 1, 2019

@Grsmto Thanks for all the work! And sorry to ask you again, but I think only a minor change in package.json is required to get it working again, right? Would be very much appreciated to get it working again 😃

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

Oops 😨 e0838e4 CI test failed ❗

@Grsmto please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/25642 for details 📝 !
Most of the error will have corresponding explanation, so that you will know what's wrong and then try to fix it!
If you cannot understand the error message and need help, feel free to ask our maintainers ☺️

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@Grsmto congratulations! 1925476 CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/25644, thank you 😀

@olafcm
Copy link
Contributor

olafcm commented Feb 5, 2019

@extend1994 Can you please help to have the pull request merged so the lib is being updated on cdnjs again? Thanks! 👍 👍

@PeterDaveHello
Copy link
Contributor

@olafcm let me take a look 👍

@@ -172,7 +172,7 @@
return store[key] || (store[key] = value !== undefined ? value : {});
})('versions', []).push({
version: _core.version,
mode: 'global',
mode: _library ? 'pure' : 'global',
Copy link
Contributor

Choose a reason for hiding this comment

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

This file was added from the previous commit, which also claims it's the latest, it's weird to be updated again here, is there anything I missed here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Grsmto would you please take a look? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea why this was updated.
To be honest I spent so so so much time on this PR, I think I'm about to give up. Every time I need to update something I spend at least an hour with the process in here :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I double checked and the file are 1:1 with the npm version. It was a mistake from a while back. As I'm hard pushing it looks like it's from the latest commit but it's not.

However of course in between I released a new version of SimpleBar (3.1.3) so I need to add that version as well before this can be merged.......

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PeterDaveHello ok one more try here! I added the latest version of SimpleBar 3.1.3.
Hope this is good to merge 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's a little bit hard working on a huge repository, and thanks for your time on it! To provide the best integrity and reliable service, we just need to make sure the files are correct, that's why I take a deeper look into the pull request. I'll take another check here soon to see if everything is okay.

BTW, not sure if you enabled sparse checkout, which will let you not spend so much time on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it should be at b25f999ec3787872e45ba531fe1467d8b60160c4 or bbb10c7a3af24a7b1d505851ae14e323efc2826a, one of them contains the wrong files, I'll check that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the later commit is correct, the previous isn't.

As the wrong history here is no needed in the repositoy, do you mind to do a little final revise to fix that? It'll be even better to mention the exact version in the commit message, instead of the "latest" version, as when you need to reace the update history here(Fix v3.1.1 in the previous commit, or, simple suqash that two commit of adding new versions) and see many of the latest, the more precise the more clear.

Thanks again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Squashed everything under 1 commit which was the original commit message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, the preferred form here is to separate the config part and the assets in the individual commit, the reason why I mention the squash is that the previous one of the assets adding commit was containing wrong content, which was fixed in the later commit, then a suqash can help you make it disappear in that situation, anyway, I'll try to make this one easier, get it merged ASAP, thanks again!

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

Oops 😨 0328edb CI test failed ❗

@Grsmto please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/25707 for details 📝 !
Most of the error will have corresponding explanation, so that you will know what's wrong and then try to fix it!
If you cannot understand the error message and need help, feel free to ask our maintainers ☺️

Copy link
Contributor

@PeterBot PeterBot left a comment

Choose a reason for hiding this comment

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

@Grsmto congratulations! 9c499a8 CI test passed! ✅
Please wait for the further review from the maintainers!

For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/25724, thank you 😀

@olafcm
Copy link
Contributor

olafcm commented Feb 7, 2019

@PeterDaveHello Thank you kindly for your support. Is it ready for final merge now? I see v3.1.0 is hosted already (
https://cdnjs.com/libraries/simplebar/3.1.0).

@PeterDaveHello PeterDaveHello changed the title [author] Update SimpleBar after folder structure change [author] Update SimpleBar v3.1.x with new auto-update config Feb 7, 2019
@PeterDaveHello PeterDaveHello merged commit 2d71042 into cdnjs:master Feb 7, 2019
@ghost ghost removed the in progress label Feb 7, 2019
@PeterDaveHello
Copy link
Contributor

Sure! Merged!

@olafcm
Copy link
Contributor

olafcm commented Feb 7, 2019

Working perfect, thank you so much @PeterDaveHello and @Grsmto!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants