PDPDEVTOOL-6272: Create UI to provide Feedback Loop for DevAssist #971
Conversation
oracle-contributor-agreement
bot
added
the
OCA Verified
…op-UI # Conflicts: # packages/vscode-extension/messages.json # packages/vscode-extension/src/service/TranslationKeys.ts
| <form class="content" id="feedbackForm"> | ||
| <div class="row"> | ||
| <label for="feedback">Your Feedback</label> | ||
| <textarea id="feedback" name="feedback" |
There was a problem hiding this comment.
Maybe you should add the maxlength property
There was a problem hiding this comment.
I like this proposal.
If we showed the remaining characters available count on the textarea (such that we display 0 when maxLength is archieved) then maybe this would be a better experience (HTML wouldn't let you type further).
But this isn't the case and I think the current Alert is more User Friendly.
Let's take this to Kostya and see what he thinks.
| } | ||
| }, | ||
| FEEDBACK_FORM: { | ||
| VALIDATION_ERROR: 'DEVASSIST_SERVICE_FEEDBACK_FORM_VALIDATION_ERROR', |
There was a problem hiding this comment.
maybe you can create a group for generic errors
There was a problem hiding this comment.
I renamed the message to make it clearer. Let me know what you think
| // Send request to NetSuite Backend through Proxy | ||
| try { | ||
| const currentProxySettings = getDevAssistCurrentSettings(); | ||
| const response = await fetch(`http://127.0.0.1:${currentProxySettings.localPort}/api/internal/devassist/feedback`, { |
There was a problem hiding this comment.
The path can be a constant value into the class file
There was a problem hiding this comment.
Reused the ApplicatinConstants instead of writing URL directly.
This change is better for maintainability, but is less clear IMO. Let me know if it's okay
| vsLogger.error("Feedback Form External Failure: " + response.status + ' ' + response.statusText); | ||
| vsLogger.error(''); | ||
|
|
||
| if (response.status === 403) { |
There was a problem hiding this comment.
It can be a constant value, i.e. HTTP_FORBIDDEN = 403
There was a problem hiding this comment.
Added HTTP_STATUS to Node Application constants and reused it here as well as in the SuiteCLoudAuthProxyService.js
| } | ||
| } | ||
| } catch (e) { | ||
| vsLogger.printTimestamp(); |
There was a problem hiding this comment.
I am not very fond of large lines of code inside an exception catch. Maybe (optional) can you put it inside a method so does not look like a flow control instead of an exception
|
|
||
| const handleWebviewMessage = async (webviewMessage : any, feedbackFormCSSFilePath : string) : Promise<void> => { | ||
| switch (webviewMessage.type) { | ||
| case WEBVIEW_EVENTS.SUBMIT_FEEDBACK: |
There was a problem hiding this comment.
I see too much code into sentences swich case that should be brief. Maybe you can wrap the lines of code for each case into functions.
| NOTIFICATION_BUTTON: 'DEVASSIST_SERVICE_IS_RUNNING_NOTIFICATION_BUTTON', | ||
| OUTPUT: 'DEVASSIST_SERVICE_IS_RUNNING_OUTPUT', | ||
| STATUSBAR: 'DEVASSIST_SERVICE_IS_RUNNING_STATUSBAR' | ||
| }, |
There was a problem hiding this comment.
I know we already have "See Details" button somewhere else, but could we bring it here and add the "Give Feedback" one into a button section.
I have 2 proposals (and both could leave out the "See Details" button if you don't want to repeat it here)
Option A:
IS_RUNNING: {
BUTTON: {
GIVE_FEEDBACK: 'DEVASSIST_SERVICE_IS_RUNNING_BUTTON_GIVE_FEEDBACK',
SEE_DETAILS: 'DEVASSIST_SERVICE_IS_RUNNING_BUTTON_SEE_DETAILS'
},
NOTIFICATION: 'DEVASSIST_SERVICE_IS_RUNNING_NOTIFICATION',
OUTPUT: 'DEVASSIST_SERVICE_IS_RUNNING_OUTPUT',
STATUSBAR: 'DEVASSIST_SERVICE_IS_RUNNING_STATUSBAR'
}
Option B:
IS_RUNNING: {
NOTIFICATION: {
BUTTON: {
GIVE_FEEDBACK: 'VALUE',
SEE_DETAILS: 'VALUE'
},
MESSAGE: 'VALUE'
},
OUTPUT: 'DEVASSIST_SERVICE_IS_RUNNING_OUTPUT',
STATUSBAR: 'DEVASSIST_SERVICE_IS_RUNNING_STATUSBAR'
}
Another option could be to just change NOTIFICATION for NOTIFICATION_MESSAGE . I'm open to any other approach you can think about, but I wanted somehow keep coherence among most of the different translation keys present in this file.
| const infoMessage: string = translationService.getMessage(DEVASSIST_SERVICE.IS_RUNNING.NOTIFICATION, proxyUrl); | ||
| const buttonsAndActions: { buttonMessage: string, buttonAction: () => void }[] = [ | ||
| { | ||
| buttonMessage: translationService.getMessage(BUTTONS.SEE_DETAILS), | ||
| buttonAction: vsNotificationService.showOutput | ||
| }, | ||
| { | ||
| buttonMessage: translationService.getMessage(DEVASSIST_SERVICE.IS_RUNNING.NOTIFICATION_BUTTON), | ||
| buttonAction: () => { | ||
| vscode.commands.executeCommand('suitecloud.opendevassistfeedbackform') | ||
| } | ||
| }, | ||
| ]; | ||
| vsNotificationService.showCommandInfoWithSpecificButtonsAndActions(infoMessage, buttonsAndActions); |
There was a problem hiding this comment.
Can we extract this logic to a method to improve code readability?
I have some suggestions:
showDevAssistIsRunningNotificationshowDevAssistStartNotificationshowStartServiceNotificationshowStartDevAssistNotificationshowDevAssistStartedNotification
I'm kind of asking to replicate what it is done on showDevAssistStartUpNotification.
3 participants
On Code Review:
Missing from the code review