-
Notifications
You must be signed in to change notification settings - Fork 25
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
Adding Usage Report area #1746
Adding Usage Report area #1746
Conversation
5637f26
to
0ab0828
Compare
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
fde9414
to
5c7ca66
Compare
8b1c4e9
to
30a2a5a
Compare
…erQueuesView.vue Co-authored-by: Jo Palac <jpalac@users.noreply.github.com>
…View.vue Co-authored-by: Jo Palac <jpalac@users.noreply.github.com>
…iew.vue Co-authored-by: Jo Palac <jpalac@users.noreply.github.com>
…iew.vue Co-authored-by: Jo Palac <jpalac@users.noreply.github.com>
…iew.vue Co-authored-by: Jo Palac <jpalac@users.noreply.github.com>
…iew.vue Co-authored-by: Jo Palac <jpalac@users.noreply.github.com>
…iew.vue Co-authored-by: Jo Palac <jpalac@users.noreply.github.com>
src/Frontend/src/views/throughputreport/endpoints/DetectedEndpointsView.vue
Outdated
Show resolved
Hide resolved
…ointsView.vue Co-authored-by: Jo Palac <jpalac@users.noreply.github.com>
@@ -35,7 +35,7 @@ body { | |||
display: flex; | |||
flex-direction: column; | |||
height: 100vh; | |||
min-width: 640px; | |||
min-width: 930px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a big increase - this min-width was a (temporary) fix to ensure all the menu items still show when on a smaller resolution viewport. Isn't it just a single extra icon, about 50px including space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
temporary because ideally we'd switch to a hamburger menu below this width, rather than showing a horizontal scrollbar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a different setting from the top bar, https://github.com/Particular/ServicePulse/pull/1746/files#diff-19ae39d4ffb6b88034dc79db811f4b812e14e71c20e6f2f9a2fd598f19ec3860L172.
The intention here is to ensure the new screen layout renderers correctly, given it is a large table.
|
||
export const useThroughputStore = defineStore("ThroughputStore", () => { | ||
const testResults = ref<ConnectionTestResults | null>(null); | ||
const refresh = () => dataRetriever.executeAndResetTimer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const refresh = () => dataRetriever.executeAndResetTimer(); | |
const refresh = dataRetriever.executeAndResetTimer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
return testResults.value.transport as Transport; | ||
}); | ||
const isBrokerTransport = computed(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would seem safer if the switch defaulted to false rather than true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the majority of our transports are "brokers", I thought it is more logical to default to true
|
||
async function generateReport() { | ||
const results = await throughputClient.endpoints(); | ||
const found = results.find((value) => !value.user_indicator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const found = results.find((value) => !value.user_indicator); | |
const hasNonUserIndicatorEndpoint = results.find((value) => !value.user_indicator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed
const results = await throughputClient.endpoints(); | ||
const found = results.find((value) => !value.user_indicator); | ||
|
||
if (found) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (found) { | |
if (hasNonUserIndicatorEndpoint ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or just use the results.find directly in the if clause
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
</ConnectionResultView> | ||
<ConnectionResultView v-if="useIsMonitoringEnabled()" title="Monitoring" :result="testResults?.monitoring_connection_result!"> | ||
<template #instructions> | ||
<a href="https://docs.particular.net/servicecontrol/monitoring-instances/installation/creating-config-file" target="_blank">Learn how to configure monitor instances</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check: are there any links in servicepulse that open in a new tab that (correctly) have the icon next to them indicating that they will open in a new tab? Something similar to this should be used here, at the end of the text https://www.iconfinder.com/icons/192090/external_go_to_hyperlink_link_new_tab_outside_url_icon#size=128 (note that this icon has been used to indicate "external link" rather than "opens in new tab"
<div class="col-6"> | ||
<div> | ||
<p> | ||
The report that is generated will contain the names of endpoints/queues.<br /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be using <br>
fileName = parse(contentDisposition).parameters["filename"] as string; | ||
} | ||
} catch { | ||
//do nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better explanation? Is the parse expected to fail for some headers, and then we are happy to keep the default fileName set above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
try { | ||
if (contentDisposition != null) { | ||
parse(contentDisposition); | ||
fileName = parse(contentDisposition).parameters["filename"] as string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double parse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
public async getMasks() { | ||
if (isThroughputSupported.value) { | ||
const [, data] = await useTypedFetchFromServiceControl<string[]>(`${this.basePath}/settings/masks`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting is wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
I haven't addressed the px vs em and breaks |
Part of #1888