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

Don’t merge request number and url. #121

Merged
merged 2 commits into from Jan 15, 2017
Merged

Don’t merge request number and url. #121

merged 2 commits into from Jan 15, 2017

Conversation

tobli
Copy link
Collaborator

@tobli tobli commented Jan 15, 2017

Put request number in a separate element, so request url is shown unchanged (in hover and title). Right aligns the request number, and aligns the start position of all urls. Additionally reorders some code in row svg creation.

Put request number in a separate <text> element, so request url is shown unchanged (in hover and title). Right aligns the request number, and aligns the start position of all urls. Additionally reorders some code in row svg creation.
@tobli
Copy link
Collaborator Author

tobli commented Jan 15, 2017

before

before: note the inclusion of request number in the <title>, and that the url shifts to the right from 9 to 10.

after

after: request number is now separate, and written right align in a space that's wide enough to fit all numbers.

@tobli
Copy link
Collaborator Author

tobli commented Jan 15, 2017

@micmro and @soulgalore - I need some input on this. I'm separating request number into it's own element, which I think is good. However some of the other changes might be controversial, and should maybe to broken out. The layout still doesn't look good for HARs with hundreds of entries, especially not if at least one entry has multiple warning triangles (e.g. www.dn.se). I'd propose some additional changes enabled by this separation:

  • collapse all warnings into one symbol (showing details on hover, and when expanding the overlay).
  • put request number to the far left (and configurable)
  • swap warning and mime icons (so mime and url are directly next to each other).

@micmro
Copy link
Owner

micmro commented Jan 15, 2017

  • collapse all warnings into one symbol (showing details on hover, and when expanding the overlay).

That's a pretty elegant solution - really like this. 👍

  • put request number to the far left (and configurable)

Works for me

  • swap warning and mime icons (so mime and url are directly next to each other).

No objects to this, I've just create a mockup, just to see what it looks like:

Before:
screen shot 2017-01-15 at 5 01 42 pm

After (not sure with the first warning sign if we should move it as well):
dummy-tobias

@soulgalore
Copy link
Collaborator

This looks great!

After (not sure with the first warning sign if we should move it as well):

Maybe the first one should have a ? Or we move it in, it will probably look better.

@@ -16,6 +16,21 @@ import * as generalComponents from "./sub-components/svg-general-components";
import * as marks from "./sub-components/svg-marks";

/**
* Get a string that's as wide, or wider than any number from 0-n.
* @param n
Copy link
Owner

@micmro micmro Jan 15, 2017

Choose a reason for hiding this comment

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

can you just add a type for this?
@param {number} n

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, I'll fix that. Will check for a lint rule as well.

@tobli
Copy link
Collaborator Author

tobli commented Jan 15, 2017

My idea for the main icon is to view it as "response status" that covers mime type (for 2xx replies), redirect (for 3xx) and errors (for 4/5xx). Anything else (e.g. missing gzip) is a warning that's listed in the additional icon column.

@tobli tobli merged commit 0adf2ae into master Jan 15, 2017
@tobli tobli deleted the split-request-number branch January 15, 2017 09:23
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