Skip to content

Initial checkin - #1

Merged
Rob Paveza (robpaveza) merged 22 commits into
masterfrom
initial-checkin
Jun 10, 2020
Merged

Initial checkin#1
Rob Paveza (robpaveza) merged 22 commits into
masterfrom
initial-checkin

Conversation

@robpaveza

Copy link
Copy Markdown
Contributor

Network Console is an experimental UI for creating synthetic HTTP requests from DevTools. It is designed for web API developers and consumers, and is intended to be compatible with similar tools.

Network Console is an experimental UI for creating synthetic HTTP
requests from DevTools. It is designed for web API developers
and consumers, and is intended to be compatible with similar tools.
In a move to migrate this to a single-file distribution, a necessary
component was to add CSP to this application. We've now identified
all the locations from which we load web-based sources. A future
version may move all of the dependencies directly into the
application.

Also removed a now-unused build script.
Comment thread .gitignore Outdated
Comment thread packages/devtools-network-console/README.md Outdated
Comment thread README.md Outdated
Comment thread packages/devtools-network-console/src/actions/modal/index.ts Outdated
Comment thread packages/devtools-network-console/src/actions/request/body.ts Outdated
Comment thread packages/network-console-shared/src/index.ts Outdated
Comment thread packages/network-console-shared/src/net/verb.ts Outdated
Comment thread packages/network-console-shared/src/util/lazy.ts Outdated
Comment thread scripts/tasks/run-process.js Outdated
Comment thread scripts/tasks/stage-frontend-output.js Outdated

@joselea Jose Leal (joselea) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There are many files so it will take me a while. Initial set of comments

Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread packages/devtools-network-console/public/index.html Outdated
Comment thread packages/devtools-network-console/public/index.html
Comment thread packages/devtools-network-console/public/index.html Outdated
Comment thread packages/devtools-network-console/src/App.tsx Outdated
Comment thread packages/devtools-network-console/src/actions/collections/index.ts Outdated
Comment thread packages/devtools-network-console/src/actions/request/body.ts Outdated
Comment thread scripts/tasks/run-process.js Outdated
Comment thread scripts/tasks/build-frontend-component.js

@xirzec Jeff Fisher (xirzec) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left some thoughts, overall the project seems reasonable and well organized. Mostly nits and minor polish stuff.

Cool tool, happy to see React doing some work for you. Curious how good/bad your devloop was.


it('renders without crashing', () => {
const div = document.createElement('div');
ReactDOM.render(<App />, div);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe should be rendering this inside a Strict tag for extra checks?

Comment thread packages/devtools-network-console/public/index.html
Comment thread packages/devtools-network-console/src/App.tsx Outdated
Comment thread packages/devtools-network-console/package.json
Comment thread packages/devtools-network-console/package.json
Comment thread packages/devtools-network-console/src/ui/Authorization/index.tsx
Comment thread packages/devtools-network-console/src/ui/Authorization/index.tsx
);
}

const AUTH_SCHEME_NAME_MAP = new Map<NetworkConsoleAuthorizationScheme, string>([

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe these maps shouldn't live in the ux component?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why? They present UX strings (or the earlier map associates a value to a UX component index).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well presumably you'd maybe want to localize them somewhere else and I could see multiple components wanting to use the strings in the future. Maybe that's premature, it just feels weird to have only this component ever know about how these values translate to something readable.

Comment thread packages/devtools-network-console/src/utility/assert.ts Outdated
Comment thread packages/network-console-shared/src/util/base64.ts
Primarily, these are:
 - Build script comments describing what is done
 - React style and nits

@joselea Jose Leal (joselea) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a very long list of files, understandable since its the initial port of files. I couldn't find anything wrong that stands up other that what we discussed in the comments. I'm approving with the understanding that this project will continue to be develop and that next PRs will be smaller.

@bgoddar Brandon Goddard (bgoddar) left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just a few nits here and there. Looks like a couple of places in your actions and reducers had some random empty lines added that don't seem intentional.

Didn't spend much time reviewing the UI sections either, let me know if you want me to go back, but based on the last local test I did I don't think I would find much that isn't already mentioned

Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread scripts/install-and-build.js Outdated
Comment thread scripts/tasks/deploy-to-chromium.js
Comment thread packages/devtools-network-console/src/actions/modal/index.ts Outdated
Comment thread packages/devtools-network-console/src/actions/request/query.ts Outdated
Comment thread packages/devtools-network-console/src/actions/request/route.ts Outdated
Comment thread packages/devtools-network-console/src/data/index.ts
Comment thread packages/devtools-network-console/src/utility/download.ts
 - Changed the title editor to be editable on click (always a text box)
 instead of via a button
 - Removed the splitter button (Send and Download) and moved Download
 functionality into the response viewer
 - Moved Computed URL and response stats into Status Bar views in
 the request and response sides
 - Split off the address bar components so they don't touch, and restored
 their rounded corners
 - Matched FAST styling for the editor grids - removed vertical
 gridlines
 - Removed color for response status
 - Vertical splitter resize bar - switched to gray and removed the
 % difference overlay
- "Create new request" does not create a tab for it
- Verb picker should not be free-form

Dark theme:
 - Links should be consistently styled
 - "These parameters may contain sensitive information" in authorization warning is white on yellow. Violates CC requirements.
Also corrected a bug in which HTML previews wouldn't be displayed
because they were blocked by CSP or the HTML content was too long.
@robpaveza
Rob Paveza (robpaveza) merged commit ad31640 into master Jun 10, 2020
@robpaveza
Rob Paveza (robpaveza) deleted the initial-checkin branch June 10, 2020 20:15
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