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.

Sign up for GitHub

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

Jump to bottom

feat: add HTML Renderer #193

Merged
merged 30 commits into from
Aug 27, 2024
Merged

feat: add HTML Renderer #193

merged 30 commits into from
Aug 27, 2024

Conversation

nytamin
Copy link
Contributor

@nytamin nytamin commented Jul 11, 2024 •
edited
Loading

About Me

This pull request is posted on behalf of the NRK.

Type of Contribution

This is a: Feature

New Behavior

This PR adds an Electron-based HTML Renderer.

The HTML renderer itselt is a standalone app, which is spun up by the worker to do its rendering.

The HTML-Renderer can render any web page, and take snapshot images and recordings of them.

Other Information

Note: this PM is dependent of the types added in Core.

Status

  • PR is ready to be reviewed.
  • The functionality has been tested by the author.
  • Relevant unit tests has been added / updated.
  • Relevant documentation (code comments, system documentation) has been added / updated.

@nytamin nytamin mentioned this pull request Jul 11, 2024
Closed
7 tasks
@nytamin nytamin requested a review from a team July 11, 2024 13:09
jstarpl
jstarpl requested changes Aug 12, 2024
View reviewed changes
Copy link
Member

@jstarpl jstarpl left a comment

Choose a reason for hiding this comment

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

This is a bit difficult to follow for me. Maybe we could schedule some pair-programming time for this?

apps/html-renderer/app/README.md Outdated Show resolved Hide resolved
apps/html-renderer/packages/generic/src/renderHTML.ts Outdated Show resolved Hide resolved
apps/html-renderer/packages/generic/src/renderHTML.ts Outdated Show resolved Hide resolved
apps/html-renderer/packages/generic/src/renderHTML.ts Show resolved Hide resolved
apps/html-renderer/packages/generic/src/renderHTML.ts Outdated Show resolved Hide resolved
apps/html-renderer/packages/generic/src/renderHTML.ts Outdated Show resolved Hide resolved
apps/html-renderer/packages/generic/src/renderHTML.ts Outdated Show resolved Hide resolved
@nytamin nytamin mentioned this pull request Aug 21, 2024
Merged
7 tasks
nytamin added 5 commits August 23, 2024 07:45
@nytamin
fix: remove outputPrefix for html-template expectations
43e9887
@nytamin
chore: move some helper functions so that they can be shared with Sof…
9a2d6fd 
…ie Core
@nytamin
chore: lint
d342bc7
@nytamin
Merge branch 'master' into feat/html-rendering
e964533 
# Conflicts:
#	apps/package-manager/packages/generic/src/generateExpectations/nrk/expectations.ts
#	package.json
#	shared/packages/worker/package.json
#	shared/packages/worker/src/worker/workers/genericWorker/genericWorker.ts
#	yarn.lock
@nytamin
chore: update code dep
77a7a28
@Julusian
Copy link
Contributor

I think this needs some work on DX
In order for the html-renderer to run, you must produce the deployment version of html-renderer. This is not documented. It would be even nicer if it would run the code version, to avoid needing to run the deploy. (particularly painful on linux, as there is no command to produce the deploy version)

If the takeScreenshot operation is missing a file extension, the operation fails. Should this choose an extension automatically, or could it produce a better error?

@nytamin
Copy link
Contributor Author

nytamin commented Aug 26, 2024

I think this needs some work on DX In order for the html-renderer to run, you must produce the deployment version of html-renderer. This is not documented. It would be even nicer if it would run the code version, to avoid needing to run the deploy.

Good point! I've pushed a commit just now so that it'll use the code version when in development mode.

If the takeScreenshot operation is missing a file extension, the operation fails. Should this choose an extension automatically, or could it produce a better error?

I've pushed a commit just now to default to png. 👍

@nytamin nytamin merged commit e576a66 into master Aug 27, 2024
6 checks passed
@nytamin nytamin deleted the feat/html-rendering branch August 27, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jstarpl jstarpl jstarpl requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nytamin @Julusian @jstarpl