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

feat(image): add image dataUri with base64 #2273

Merged
merged 17 commits into from Sep 5, 2023
Merged

feat(image): add image dataUri with base64 #2273

merged 17 commits into from Sep 5, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jul 23, 2023

Adds a image type parameter type to faker.image.dataUri.

If type equals 'svg', it returns a URI that contains a svg image as before.
If type equals 'base64', it returns a URI that contains a base64 image as described in #1550.

Currently the base64 image is generated by converting a svg image. I am not sure, if that is an issue. As @matthewmayer pointed out in the issue, it would probably bloat the code if it should also return a base64 encoded jpg or png.

Additionally I added a few tests for the new parameter similar to the ones that already existed.

Closes #1550.

@ghost ghost self-requested a review as a code owner July 23, 2023 10:25
@codecov
Copy link

codecov bot commented Jul 23, 2023

Codecov Report

Merging #2273 (a4bd63c) into next (f71cc1c) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2273      +/-   ##
==========================================
- Coverage   99.61%   99.60%   -0.01%     
==========================================
  Files        2774     2774              
  Lines      251763   251780      +17     
  Branches     1083     1082       -1     
==========================================
+ Hits       250783   250788       +5     
- Misses        953      965      +12     
  Partials       27       27              
Files Changed Coverage Δ
src/modules/image/index.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

Alex and others added 2 commits July 23, 2023 12:51
…base64') because it is available in node-14 and can handle characters beyond the ASCII range
@ST-DDT ST-DDT assigned ghost Jul 31, 2023
@ST-DDT ST-DDT added c: feature Request for new feature p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision labels Jul 31, 2023
@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Aug 3, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Aug 3, 2023

There seems to be merge conflicts. Could you please fix them?

@ghost
Copy link
Author

ghost commented Aug 8, 2023

Sorry for the late reply. I fixed the merge conflicts.

@xDivisionByZerox xDivisionByZerox removed the needs rebase There is a merge conflict label Aug 8, 2023
@xDivisionByZerox
Copy link
Member

Team decision:

We accept this feature. This can now be reviewed.

@xDivisionByZerox xDivisionByZerox added s: accepted Accepted feature / Confirmed bug and removed s: needs decision Needs team/maintainer decision labels Aug 10, 2023
@xDivisionByZerox xDivisionByZerox requested a review from a team August 10, 2023 16:13
src/modules/image/index.ts Outdated Show resolved Hide resolved
src/modules/image/index.ts Outdated Show resolved Hide resolved
src/modules/image/index.ts Outdated Show resolved Hide resolved
ST-DDT
ST-DDT previously approved these changes Aug 14, 2023
@ST-DDT ST-DDT requested a review from a team August 14, 2023 07:11
@ST-DDT ST-DDT requested a review from a team August 14, 2023 07:11
src/modules/image/index.ts Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Member

ST-DDT commented Aug 14, 2023

I just noticed, that dataUri doesn't have seeded tests, so I created a PR for that.

Blocked by:

@ST-DDT ST-DDT added the s: on hold Blocked by something or frozen to avoid conflicts label Aug 14, 2023
@ST-DDT ST-DDT removed the s: on hold Blocked by something or frozen to avoid conflicts label Aug 17, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Aug 17, 2023

We added some (forgotten) seeded tests to ensure the values generated by this method are reproducible.
Please extend them to include the type option.

t.describe('dataUri', (t) => {
t.it('noArgs')
.it('with width', { width: 128 })
.it('with height', { height: 128 })
.it('with width and height', { width: 128, height: 128 })
.it('with color', { color: 'blue' })
.it('with all options', {
width: 128,
height: 128,
color: 'blue',
});
});

test/modules/image.spec.ts Show resolved Hide resolved
src/modules/image/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Sorry, for all the back and forth.

src/modules/image/index.ts Outdated Show resolved Hide resolved
test/modules/image.spec.ts Outdated Show resolved Hide resolved
test/modules/image.spec.ts Outdated Show resolved Hide resolved
src/modules/image/index.ts Show resolved Hide resolved
src/modules/image/index.ts Outdated Show resolved Hide resolved
src/modules/image/index.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT requested review from a team August 21, 2023 06:17
@ST-DDT ST-DDT requested a review from matthewmayer August 28, 2023 18:31
@ST-DDT ST-DDT enabled auto-merge (squash) September 5, 2023 07:09
@ST-DDT ST-DDT merged commit 869b9b4 into faker-js:next Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add image dataUri with base64
4 participants