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

Update output.js #2581

Merged
merged 3 commits into from
Feb 17, 2021
Merged

Update output.js #2581

merged 3 commits into from
Feb 17, 2021

Conversation

florian-busch
Copy link
Contributor

This is my first pull request (at all!). Please let me know if something needs improvement.

I added a const formatLowerCase at line 190 that saves the lowerCase version of the format parameter.
At 192 and 194 I replaced "format" with "formatLowerCase.

This is my first request so I'm not quite sure about the conventions. Please let me know if something needs improvement.
I added a const formatLowerCase at line 190 that saves the lowerCase version of the format parameter. 
At 192 and 194 I replaced "format" with "formatLowerCase.
Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

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

Hi, thank you very much for the PR.

Please can you run npm test locally to ensure all the linting rules and unit tests still pass. If you could also add a test to cover this new feature e.g. .toFormat('JPEG') that would be even better.

I've also left one comment inline that will need addressing.

lib/output.js Outdated Show resolved Hide resolved
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f49eabc on florian-busch:patch-1 into 2020839 on lovell:master.

@florian-busch
Copy link
Contributor Author

Updated output.js to accept upper case characters.
Created test for string and object input toFormat().
All tests work locally. Please let me know if other problems occur

@lovell lovell merged commit df7b8ba into lovell:master Feb 17, 2021
@lovell
Copy link
Owner

lovell commented Feb 17, 2021

Brilliant, thank you very much.

@lovell lovell added this to the v0.27.2 milestone Feb 17, 2021
@florian-busch florian-busch deleted the patch-1 branch February 18, 2021 08:01
lovell added a commit that referenced this pull request Feb 19, 2021
@lovell
Copy link
Owner

lovell commented Feb 22, 2021

v0.27.2 now available, thanks for the PR.

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

3 participants