Skip to content
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

New: Add the Otp Modal #619

Open
wants to merge 2 commits into
base: feature/one-zerop-bank
Choose a base branch
from

Conversation

shaiu
Copy link
Contributor

@shaiu shaiu commented Nov 10, 2024

No description provided.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 suggestion.

Files not reviewed (1)
  • packages/renderer/src/components/GetOtp.module.css: Language not supported
Comments skipped due to low confidence (1)

packages/renderer/src/components/GetOtp.tsx:49

  • [nitpick] The button text is in Hebrew. Please confirm that this is intentional and correct.
הכנס קוד

@@ -91,3 +91,7 @@ export async function electronGoogleOAuth2Connector(): Promise<Credentials> {
export async function createSpreadsheet(spreadsheetId: string, credentials: Credentials): Promise<string> {
return electron.ipcRenderer.invoke('createSpreadsheet', spreadsheetId, credentials);
}

export async function sendUserInput(input: string) {
Copy link
Preview

Copilot AI Nov 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function sendUserInput lacks error handling, which could lead to unhandled promise rejections. Consider adding a try-catch block.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@baruchiro all other methods don't have error handling at all, so I don't think we should start now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

הקופיילוט די גרוע, תתעלם

Copy link
Collaborator

@baruchiro baruchiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the issues and I will review it again.

Copy link
Collaborator

@baruchiro baruchiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now with the comments

Comment on lines 95 to 97
export async function sendUserInput(input: string) {
await electron.ipcRenderer.send('get-otp-response', input);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export async function sendUserInput(input: string) {
await electron.ipcRenderer.send('get-otp-response', input);
}
export async function sendOTPResponse(input: string) {
await electron.ipcRenderer.send('get-otp-response', input);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed

@@ -91,3 +91,7 @@ export async function electronGoogleOAuth2Connector(): Promise<Credentials> {
export async function createSpreadsheet(spreadsheetId: string, credentials: Credentials): Promise<string> {
return electron.ipcRenderer.invoke('createSpreadsheet', spreadsheetId, credentials);
}

export async function sendUserInput(input: string) {
await electron.ipcRenderer.send('get-otp-response', input);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why send and not invoke?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still learning. What's the difference?

Comment on lines 11 to 14
const [modalStatus, setModalStatus] = useState<ModalStatus>(ModalStatus.HIDDEN);
const [inputText, setInputText] = useState('');

const closeModal = () => setModalStatus(ModalStatus.HIDDEN);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ModalStatus is to reuse the main modal (Body.tsx) for different usages.

Here you create a new Modal, I think you can just use a boolean state to decide if the Modal is open or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. changed to boolean

@shaiu shaiu requested a review from baruchiro November 11, 2024 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants