-
Notifications
You must be signed in to change notification settings - Fork 28
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
feat: add parameter to disable path encoding #124
feat: add parameter to disable path encoding #124
Conversation
Features
ContributorsCommit-Lint commandsYou can trigger Commit-Lint actions by commenting on this PR:
|
5be58cd
to
a140441
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @pypas,
While this looks good at first, there's one or two things we want to double check. Nonetheless, thank you for your hard work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @pypas - looking into it a little deeper, it looks like your changes broke some of our preexisting tests as is. While your revised tests do pass as intended, there's a concern that this might serve as a breaking change for current users. In this instance, it'd be ideal for a solution to this issue to be able to pass our current tests as is, in addition to any new ones you've written testing the new functionality.
581c6bc
to
68816c8
Compare
Hey @atlawrie , I've updated the code as not to break the preexisting test, let me know what you think! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @pypas,
This is looking great, awesome work.
I do think we need to enable this functionality for the srcsets as well. Do you think you could update this PR to include the disable_path_encoding
option when to_url
in invoked in build_srcset_pairs and build_dpr_srcset?
edit: I forgot to mention, we'll need tests for disable_path_encoding
in the to_srcset
specs as well.
68816c8
to
ede1d53
Compare
README.md
Outdated
#=> https://your-subdomain.imgix.net/%5Bimages%5D/demo.png?s=270832685733a36ba02bd8ab9fd72df5 | ||
client.path('/[images]/demo.png').to_url({},{disable_path_encoding: true}) | ||
#=> https://your-subdomain.imgix.net/[images]/demo.png?s=ed6eb07e9eff3f6c8bbcc83fc4f63198 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a heads up that you're missing a closing ```
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍🏼 great work, let's just add that closing "```" to the README. I've also left two small non-blocking comments, but feel free to ignore them.
Once that README change is done we should be ready to merge.
Co-authored-by: Luis H. Ball Jr. <[email protected]>
1003de8
to
c93c409
Compare
Thanks for the suggestions @luqven and @sherwinski! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
Adds a
disable_path_encoding
parameter to allow user to opt-out of path encoding (issue: #117)Checklist