Skip to content

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

Merged
merged 10 commits into from
Aug 8, 2023

Conversation

BrunoQuaresma
Copy link
Collaborator

Screen Shot 2023-08-03 at 15 17 36

@BrunoQuaresma BrunoQuaresma requested a review from mtojek August 3, 2023 18:30
@BrunoQuaresma BrunoQuaresma self-assigned this Aug 3, 2023
@matifali
Copy link
Member

matifali commented Aug 4, 2023

to me the font size looks very small. and a lot of unused blank space.

@bpmct
Copy link
Member

bpmct commented Aug 4, 2023

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?

Copy link
Member

@mtojek mtojek left a 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:

Screenshot 2023-08-04 at 16 35 09

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 if name property is exposed instead of the value? It is better to show US (Virginia) instead of eastus.
  • booleans can be rendered as some icons instead of true/false
  • URLs could be detected and replaced with links

@BrunoQuaresma
Copy link
Collaborator Author

to me the font size looks very small. and a lot of unused blank space.

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.

@mtojek
Copy link
Member

mtojek commented Aug 4, 2023

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.

@BrunoQuaresma
Copy link
Collaborator Author

  • list(strings) can be rendered as labels/tags

Good one 👍

  • as we don't know what is inside string values, maybe we should wrap them with code tags

I don't see how this would be better 🤔

  • consider adding all table headers (value, count). The last column looks like SQL injection...

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

  • for options: [ ], can you check if name property is exposed instead of the value? It is better to show US (Virginia) instead of eastus.

Makes total sense 👍

  • booleans can be rendered as some icons instead of true/false

Totally agree 👍

  • URLs could be detected and replaced with links

Sounds good 👍

@BrunoQuaresma
Copy link
Collaborator Author

@mtojek here is the issue: #8907

@matifali
Copy link
Member

matifali commented Aug 4, 2023

for options: [ ], can you check
if name property is exposed instead of the value? It is better to show US (Virginia) instead of eastus.

@BrunoQuaresma, we also have an optional display_name, and that would be better to show instead of name. fallback to name if display_name does not exist.

@BrunoQuaresma
Copy link
Collaborator Author

@mtojek suggestions applied:

Screen.Recording.2023-08-04.at.15.10.53.mov

mtojek
mtojek previously requested changes Aug 7, 2023
Copy link
Member

@mtojek mtojek left a 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.

{data && data.length === 0 && <NoDataAvailable sx={{ height: 200 }} />}
{data &&
data.length > 0 &&
data.map((parameter, parameterIndex) => {
Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Collaborator Author

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 🤔

@mtojek
Copy link
Member

mtojek commented Aug 7, 2023

Related: #8944

@BrunoQuaresma
Copy link
Collaborator Author

With updates:

Screen.Recording.2023-08-07.at.14.05.56.mov

Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

Looks great! 👍🏻

@BrunoQuaresma BrunoQuaresma merged commit 4a987e9 into main Aug 8, 2023
@BrunoQuaresma BrunoQuaresma deleted the bq/add-parameters-usage-in-dashboard branch August 8, 2023 16:09
@github-actions github-actions bot locked and limited conversation to collaborators Aug 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants