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

ENG: Add HTML Response View #57

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

tscritch
Copy link
Contributor

@tscritch tscritch commented Nov 16, 2021

Description

Found reading the raw html particularly frustrating when creating new routes and you have error responses from Phoenix which are unreadable. I was copying the text to a file to view in the browser but this is much faster 😄

This adds an option to view a rendered html response for a route. It also adds a basic local test server for development.

The response is not rendered by default and it is rendered in an iframe for sandboxing.

Steps to test

  1. Follow steps in README.md to setup the local example folder.
  2. Go to http://localhost:3005.
  3. Try the /views/breeds.html route.
  4. Observe raw response.
  5. Click "Render HTML" button.
  6. Observe rendered response.

Screenshots

html-preview

@matthew-kerle
Copy link

Thanks for rendering this in an iframe! That was going to be my one request.

Copy link
Contributor

@jwdotjs jwdotjs 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 pretty awesome.

Thanks for adding the backend server that we can use for examples as well. I'll test this weekend and bump the tag 🙌

@jwdotjs
Copy link
Contributor

jwdotjs commented Dec 12, 2021

Screen Shot 2021-12-12 at 12 51 31 PM

I'm having trouble getting this to work when testing locally. When testing, I see a valid html response on the route I'm hitting but when I click "Render HTML", I see the above. 🤔

I can send more details over Slack if helpful.

@bjyoungblood bjyoungblood removed their request for review February 23, 2022 15:30
@tscritch
Copy link
Contributor Author

I am seeing this work for me again 🚀
Screen Shot 2022-02-24 at 12 04 43 PM

Copy link

@matthew-kerle matthew-kerle left a comment

Choose a reason for hiding this comment

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

This looks great, will test soon!

@tscritch
Copy link
Contributor Author

tscritch commented Apr 11, 2022

Just wanted to post an update posted in Slack. This should be not crashing now.
This is a video of testing with a bad route in CMW:
Screen-Recording-2022-04-06-at-11 22 20-AM

@socket-security
Copy link

Socket Security Report

Dependency issues detected. If you merge this pull request, you will not be alerted to the instances of these issues again.

📜 New install scripts detected

A dependency change in this PR is introducing new install scripts to your install step.

Package Script field Location
[email protected] (added) binding.gyp example/package.json via [email protected], [email protected]
[email protected] (added) install example/package.json via [email protected], [email protected]
[email protected] (added) postinstall example/package.json via [email protected], [email protected]
🫣 Native code

Contains native code which could be a vector to obscure malicious code, and generally decrease the likelihood of reproducible or reliable installs.

Package Location
[email protected] (added) example/package.json via [email protected], [email protected]
Socket.dev scan summary
Issue Status
Did you mean? ✅ no new possible package typos
Install scripts ⚠️ 3 new install scripts detected
Telemetry ✅ no new telemetry
Troll package ✅ no new troll packages
Malware ✅ no new malware
Native code ⚠️ 1 new native module detected
Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] [email protected]

⚠️ Please accept the latest app permissions to ensure bot commands work properly. Accept the new permissions here.

Powered by socket.dev


return (
<div>
<ApiResponseStatus status={response.status} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jwdotjs Was just fixing a merge conflict on this and wanted to be sure you intended to replace what was here with this ApiResponseStatus. It was missing from your changes here: https://github.com/smartrent/badmagic/pull/76/files#diff-1f1f2427fe79e8a1ee3be3aa06af1e81516c424970d69b021dfad9342eb26174L39

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it is getting duplicated now 🤔

Screen Shot 2022-10-07 at 2 33 47 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

spooky 👻 where is it coming from 😂
I'll remove it

Copy link
Member

@jollyjerr jollyjerr left a comment

Choose a reason for hiding this comment

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

Other than the duplicated response status this works for successful requests that return HTML. Do we want to also allow rendering for error responses?

Screen.Recording.2022-10-07.at.2.31.09.PM.mov

Comment on lines +47 to +52
// schema: {
// type: "array",
// items: {
// $ref: "#/definitions/Dog",
// },
// },
Copy link
Member

Choose a reason for hiding this comment

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

?


return (
<div>
<ApiResponseStatus status={response.status} />
Copy link
Member

Choose a reason for hiding this comment

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

Looks like it is getting duplicated now 🤔

Screen Shot 2022-10-07 at 2 33 47 PM

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.

4 participants