-
Notifications
You must be signed in to change notification settings - Fork 875
feat: add frontend for app statuses #17178
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
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.
Approving because functionally I don't have any issues with this, my comments here are mainly "would be nice if...".
}, | ||
}; | ||
|
||
const formatURI = (uri: 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.
There's a lot of util-esque functionality in here I think we should eventually extract (like the short URIs), but don't want that to block right now.
const agent = agents.find((agent) => agent.id === status.agent_id); | ||
|
||
if (currentApp && agent) { | ||
const appSlug = currentApp.slug || currentApp.display_name; |
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.
In what context would we derive the slug from the url encoded display name?
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 just copying code from how it's done before, which is weird, but I guess for backwards compat?
: theme.palette.text.disabled; | ||
switch (state) { | ||
case "complete": | ||
return <CheckCircle sx={{ color, fontSize: 18 }} />; |
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 not in love with these icons but I get this is an experiment.
} | ||
|
||
// Determine if app link should be shown | ||
const showAppLink = |
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.
Took me awhile to get the avoiding redundant links aspect of this, a comment would go a long way here I think.
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.
Will do after merge since we're in a crunch.
Check out the stories for the exacts...