-
Notifications
You must be signed in to change notification settings - Fork 11
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
fix(dashboard): better json view for TaskRun
#1112
base: master
Are you sure you want to change the base?
fix(dashboard): better json view for TaskRun
#1112
Conversation
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 you please provide screenshots for other examples that show this JSON stringify, maybe something like WfRunVariable
or ExternalEvent
output?
Because you changed it across all variable values I would like to see screenshots for those examples as well.
} else if (variable.str) { | ||
try { | ||
const json = JSON.parse(variable.str) | ||
return JSON.stringify(json, null, 4) | ||
} catch { | ||
return variable[key] | ||
} |
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.
What about variable.jsonObj
and variable.jsonArr
?
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.
Good catch
That's a good point, this same function is being used across the dashboard. So please verify every use and check if the new behavior fit on every case. |
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.
Giving it a second look, I believe this change shouldn't be implemented in this utility function but intentionally on those views where a special rendering is required.
Yeah I think we should see what it looks like on all the JSON parsing as it might be better as a default. I'm sure there are other sections of the dashboard that have JSON that needs to be formatted. 🤷♂️ We don't know if all the JSON needs special rendering, in that case we should have it as the default. |
} else if (variable.str) { | ||
try { | ||
const json = JSON.parse(variable.str) | ||
return JSON.stringify(json, null, 4) | ||
} catch { | ||
return variable[key] | ||
} |
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.
Good catch
Mijaíl 👋, I'm curious about your thought processes behind this. Is there any instance where you think we shouldn't display formatted JSON to the user? Do you think formatted JSON would bug some functionality in some way? I understand this variable util function has a lot of references, so it is hard to predict exactly how it interacts with everything, but let me know what you're thinking. |
resolves #1066
Before:
After: