-
Notifications
You must be signed in to change notification settings - Fork 937
feat(site): add parameters usage to insights #8886
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
Conversation
BrunoQuaresma
commented
Aug 3, 2023

to me the font size looks very small. and a lot of unused blank space. |
Nice! How would this work for something like "disk size" or strings that may have many values? Is a scrollbar added, or does the page get very long? |
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 the rendered table for my template, and I have a feeling that we can still apply a few improvements:

Ideas:
list(strings)
can be rendered as labels/tags- as we don't know what is inside string values, maybe we should wrap them with
code tags
- consider adding all table headers (value, count). The last column looks like SQL injection...
- for
options: [ ]
, can you check ifname
property is exposed instead of thevalue
? It is better to showUS (Virginia)
instead ofeastus
. - booleans can be rendered as some icons instead of true/false
- URLs could be detected and replaced with links
I'm going to add the description for each field, I think @mtojek is working to add the reach parameter description field into the API. |
Please file an issue for this, so we don't forget about it. |
Good one 👍
I don't see how this would be better 🤔
It does not look good, it was on the first version of the design but was removed, but I will take a look again o this option
Makes total sense 👍
Totally agree 👍
Sounds good 👍 |
…ameters-usage-in-dashboard
@BrunoQuaresma, we also have an optional |
@mtojek suggestions applied: Screen.Recording.2023-08-04.at.15.10.53.mov |
…ameters-usage-in-dashboard
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'm blocking this PR as it needs the parameter type and description, and I will prepare relevant changes.
EDIT:
Agreed with @mafredri to pass this.
site/src/pages/TemplatePage/TemplateInsightsPage/TemplateInsightsPage.tsx
Outdated
Show resolved
Hide resolved
{data && data.length === 0 && <NoDataAvailable sx={{ height: 200 }} />} | ||
{data && | ||
data.length > 0 && | ||
data.map((parameter, parameterIndex) => { |
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.
Can we wrap it into a dedicated React component, for instance, ParameterUsageRow
?
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 usually extract code into a component when:
- It has a state
- It has too many conditionals
- It is large and not easy to read
So in this case, I would not do that to avoid nested components hell as we have for the Workspace component.
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.
It is large and not easy to read
Yeah, I admit that this is my concern in terms of this PR, but maybe it isn't too hard. I saw a lot of conditional logic and assumed that it could be healthier to hide it behind a component. So, maybe not a state, but a lot of conditional logic.
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.
but a lot of conditional logic.
I'm only seeing one for the label 🤔
site/src/pages/TemplatePage/TemplateInsightsPage/TemplateInsightsPage.tsx
Outdated
Show resolved
Hide resolved
Related: #8944 |
With updates: Screen.Recording.2023-08-07.at.14.05.56.mov |
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.
Looks great! 👍🏻