-
Notifications
You must be signed in to change notification settings - Fork 1
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
✨ Added restart for app #88
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.
PR Type: Enhancement
PR Summary: This pull request introduces the functionality to restart applications directly from the user interface and through GraphQL queries. It extends the existing actions available for applications by adding a 'restart' option, which is integrated into both the frontend components and the backend GraphQL queries. The changes include modifications to UI components to accommodate the new action, updates to GraphQL queries to support restarting applications, and the necessary backend logic to execute the restart operation.
Decision: Comment
📝 Type: 'Enhancement' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
- Unsupported files: the diff contains files that Sourcery does not currently support during reviews.
General suggestions:
- Ensure consistency in iconography to improve user experience. The use of icons should intuitively reflect the action they represent. For instance, reconsider the icons chosen for the 'Restart' action to ensure they are easily understood by users.
- Review the implementation of unused functions or parameters, such as the 'vars' function in the GraphQL query for restarting apps. If they are intended for future use, adding explanatory comments might help other developers understand their purpose.
- Consider revising error messages for clarity and grammatical accuracy, ensuring they are user-friendly and professional.
- Given the addition of new functionality, it might be beneficial to review the user interface for usability and ensure that the new 'Restart' action is seamlessly integrated with the existing user experience.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
@@ -83,14 +88,26 @@ const ExtraButton = ({ onAction, item }: IExtraButton) => { | |||
options = [ | |||
{ | |||
label: 'Intercept', | |||
icon: <LinkIcon size={iconSize} />, | |||
icon: <Repeat size={iconSize} />, |
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.
suggestion (llm): The icon for the 'Restart' action seems to be a repeat icon, which might not intuitively convey the action of restarting. Consider using an icon that more closely represents the restart action to improve user experience.
options = [ | ||
{ | ||
label: 'Restart', | ||
icon: <LinkIcon size={iconSize} />, |
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.
issue (llm): It appears that the 'Restart' action is using the 'LinkIcon', which might not accurately represent the action. Consider using a more appropriate icon to enhance clarity and user experience.
|
||
const restartApp = async (item: BaseType) => { | ||
if (!environment || !project) { | ||
throw new Error('Environment is required!.'); |
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.
nitpick (llm): The error message 'Environment is required!.' contains an unnecessary punctuation mark. Consider removing the extra period for grammatical correctness.
`, | ||
{ | ||
transformer: (data: ConsoleRestartAppQuery) => data.core_restartApp, | ||
vars: (_: ConsoleRestartAppQueryVariables) => {}, |
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.
question (llm): The 'vars' function for 'restartApp' is defined but does not seem to be used. If this is intentional for future use or extensibility, consider adding a comment to clarify its purpose and avoid confusion.
No description provided.