- Notifications
You must be signed in to change notification settings - Fork906
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
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.
583a0c6 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Check out the stories for the exacts...