Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions src/pages/about/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ function About() {
return false
}

console.log('mail: ', payload.email);
if(payload.email.length < 20) {
return false
}
Comment on lines +21 to +23
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The email length validation might be too restrictive. Consider making this configurable or removing it altogether. What is the rationale behind the length check?

Suggested change
if(payload.email.length < 20) {
return false
}
// Consider removing or making this configurable
// if(payload.email.length < 20) {
// return false
// }

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Because I don’t want users to enter random emails, I want to limit the length of the email field.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for clarifying your reasoning. While I understand the intent to prevent random email entries, a simple length check might not be the most effective way to validate email format. Users could still enter long strings of gibberish that pass the length check but are not valid emails.

Consider using a more robust email validation method, such as a regular expression, or a third-party library specifically designed for email validation. This would provide a more accurate way to ensure that the entered email is in a valid format.

Suggested change
if(payload.email.length < 20) {
return false
}
// if(payload.email.length < 20) {
// return false
// }

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're welcome. I'm glad I could help. Let me know if you have any further questions.


if(payload.name === '') {
return false
}
Expand All @@ -32,9 +37,16 @@ function About() {
const {startTime, endTime, sex, name, phone} = payload
const startTimeString = format(startTime, 'yyyy/MM/dd HH:mm')
const endTimeString = format(endTime, 'yyyy/MM/dd HH:mm')

if(sex < 1 || sex > 2) {
return { success: false, result: '性別格式錯誤' }
}
const sexString = sex === 1 ? '男' : '女'

return `活動時間:${startTimeString} ~ ${endTimeString},性別:${sexString},姓名:${name},電話:${phone}`
return {
success: true,
result: `活動時間:${startTimeString} ~ ${endTimeString},性別:${sexString},姓名:${name},電話:${phone}`
}
}

function handleHallClick() {
Expand All @@ -43,7 +55,12 @@ function About() {
}

const activityString = getActivityString(mockAPIData)
// get hall data with "activityString" from api ...

if(!activityString.success) {
alert(activityString.result)
} else {
Comment on lines +59 to +61
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Consider using a more user-friendly way to display the error message instead of alert. A modal or an inline error message would be better.

// alert(activityString.result)
        // TODO: Implement a better way to display the error message
        // e.g., using a modal or an inline error message

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Due to the tight schedule, using alerts was allowed after discussion with the team.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Understood. Given the team's decision to use alerts due to the schedule, I won't push further on that point. However, for future reference and when time allows, consider exploring alternative methods for displaying error messages to enhance the user experience. Thanks for the update, @paperhsiaooo!

// get hall data with "activityString" from api ...
}
}

const handleEmailClick = () => {
Expand Down