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: display response size with a bigger unit in tooltip #2425

Merged
merged 4 commits into from
Jun 14, 2022
Merged

feat: display response size with a bigger unit in tooltip #2425

merged 4 commits into from
Jun 14, 2022

Conversation

nicognaW
Copy link

Description

The response size is always displayed using larger units like KB or MB in other API testing tools I've used. But hoppscotch shows them up with only a B. I added a tooltip to display the response size in reasonable units.

For responses with a size of 0b to 1000b, the tooltip won't show up. For those with a size of 1000b to 1000000b, kB will be shown. For those larger than 1000000b, mb will be shown. 

I am no expert on the front-end technologies, so I just imitated the current codes. If you don't like these codes, please keep the idea even if you close the pr.

@nicognaW nicognaW changed the title Display response size with a bigger unit in tooltip feat: display response size with a bigger unit in tooltip Jun 13, 2022
@AndrewBastin AndrewBastin self-requested a review June 13, 2022 06:09
Copy link
Member

@AndrewBastin AndrewBastin left a comment

Choose a reason for hiding this comment

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

@nicognaW please look into the given remarks and correct accordingly please.

Also, won't it be better if we flip the logic then ? (Show the KB/MB size data first and show bytes on hover) (cc: @liyasthomas)

Thank you for your effort by the way! ❤️

packages/hoppscotch-app/components/http/ResponseMeta.vue Outdated Show resolved Hide resolved
packages/hoppscotch-app/components/http/ResponseMeta.vue Outdated Show resolved Hide resolved
packages/hoppscotch-app/components/http/ResponseMeta.vue Outdated Show resolved Hide resolved
packages/hoppscotch-app/components/http/ResponseMeta.vue Outdated Show resolved Hide resolved
packages/hoppscotch-app/components/http/ResponseMeta.vue Outdated Show resolved Hide resolved
packages/hoppscotch-app/components/http/ResponseMeta.vue Outdated Show resolved Hide resolved
packages/hoppscotch-app/components/http/ResponseMeta.vue Outdated Show resolved Hide resolved
@nicognaW
Copy link
Author

@AndrewBastin Huge thanks for the review and suggestions. I fixed the code mentioned in the remarks and marked them resolved.

And actually, the flip logic idea also came to me the moment after I posted the PR, so I am working on it. Please give me some time🙏.

@nicognaW
Copy link
Author

I changed the logic. Now I'm seeing the result, and I think it'd be better to add another line with KB when MB is presented (the size is larger than 1000).

屏幕截图 2022-06-14 154111

In order to do that, I could only think of using allowHTML in order to render another line of text in the tooltip, which may not be an elegant implementation. Is there a better solution I could use in such a situation(despite the fact that the current display appears to be quite good to me)?

AndrewBastin
AndrewBastin previously approved these changes Jun 14, 2022
Copy link
Member

@AndrewBastin AndrewBastin left a comment

Choose a reason for hiding this comment

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

lgtm 💯

Copy link
Member

@AndrewBastin AndrewBastin left a comment

Choose a reason for hiding this comment

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

there was a minor typo I just patched. now should be fine. lgtm 💯

Thank you for the PR @nicognaW

@AndrewBastin AndrewBastin merged commit 97ff089 into hoppscotch:main Jun 14, 2022
@nicognaW
Copy link
Author

OK, thanks for your comment. I'm glad I could help.😆

liyasthomas pushed a commit that referenced this pull request Jun 16, 2022
Co-authored-by: Andrew Bastin <andrewbastin.k@gmail.com>
@nicognaW nicognaW deleted the response-size-tooltip branch June 20, 2022 07:00
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

2 participants