imgix / js-core

A JavaScript client library for generating image URLs with imgix
https://www.imgix.com
BSD 2-Clause "Simplified" License
122 stars 20 forks source link

fix: bug#340 #343

Closed FilipePfluck closed 1 year ago

FilipePfluck commented 1 year ago

Description

The library would always use encodeURI or encodeURIComponent.

This should allow users to have more freedom in specifying how URLs get encoded, rather than trying to adjust this library's default behavior to account for every edge case.

I added a test in the test-buildURL.js

Checklist

commit-lint[bot] commented 1 year ago

Features

Chore

Documentation

Contributors

luqven, FilipePfluck

Commit-Lint commands
You can trigger Commit-Lint actions by commenting on this PR: - `@Commit-Lint merge patch` will merge dependabot PR on "patch" versions (X.X.Y - Y change) - `@Commit-Lint merge minor` will merge dependabot PR on "minor" versions (X.Y.Y - Y change) - `@Commit-Lint merge major` will merge dependabot PR on "major" versions (Y.Y.Y - Y change) - `@Commit-Lint merge disable` will desactivate merge dependabot PR - `@Commit-Lint review` will approve dependabot PR - `@Commit-Lint stop review` will stop approve dependabot PR
FilipePfluck commented 1 year ago

Thanks for the reviewing! Saw the request changes, I will work on this.

FilipePfluck commented 1 year ago

Hey, I think I fixed everything you guys suggested. If there's still something for me to do, let me know.

FilipePfluck commented 1 year ago

Hey @luqven Just reminding if you didn't noticed, I updated the readme

FilipePfluck commented 1 year ago

Hey @luqven sorry for having those dist files again. I thought I removed them, but they're generated automatically, right? Perhaps I generated them again in my previous commit and didn't notice. I think that now everything is right.

luqven commented 1 year ago

Hey @FilipePfluck, sorry I should have been clearer here.

My expectation wasn't that you delete the dist files but rather that you revert the changes made to those files.

Anytime you run our build or test scripts, you'll likely update those files. But we don't stage those changes to our commits because our automated release process does it for us. Before we cut a new release, it updates the /dist folder. Our contributing guide could be clearer on this point.

I know that you've put a lot of work into this already, so I'd be more than happy to revert these myself if you grant me push permissions to your fork. Otherwise, if you feel up to it, you can revert those files to the state their in on the main branch.

Once you've done that, we should be able to merge this in.

If anything about this is unclear, please don't hesitate to reach out!

FilipePfluck commented 1 year ago

@luqven Now I understand what you meant, I just added everything with git add ., and I shouldn't have added them. Anyway, I added you as a collaborator to my fork, so feel free to revert this the way you think suits better.

luqven commented 1 year ago

Updated the branch to rollback the changes to dist/ and updated commit messages to match conventional commit specs.

Just confirming we don't need any additional tests here and we should be good to go.

FilipePfluck commented 1 year ago

Hey @luqven do we have any news about this PR? I don't mean to hurry, but if possible, I'd like to get this PR merged today, as it is the last day of october. I want to be respectful to your time as you were to mine, so if there's anything left for me to do let me know. :)

luqven commented 1 year ago

Hey @luqven do we have any news about this PR? I don't mean to hurry, but if possible, I'd like to get this PR merged today, as it is the last day of october. I want to be respectful to your time as you were to mine, so if there's anything left for me to do let me know. :)

Merging in today. Apols for delay!

imgix-git-robot commented 1 year ago

:tada: This PR is included in version 3.7.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

sherwinski commented 1 year ago

Thanks for all of your hard work on this @FilipePfluck !