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

Show alert when enter non-exist province name in the filter box. #225

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

itpcc
Copy link

@itpcc itpcc commented Mar 25, 2019

According to issue #223, I add an alert box to let the user know that the province name entered is incorrect.

I know that the alert box is not the best way to alert to the user and it needs to be improved, but IMO, it's the fastest way to do it for now.

I'm not the native React's developer, so sorry if the code isn't good enough. But I hope it'll works as it intended. (As far as I tested on Google Chrome Desktop browser, it work just fine. )

If you have any suggestion, please reply to the message. I'm glad to help and make it better.

Regards,

alert(
"ไม่พบจังหวัดที่ท่านระบุ กรุณาตรวจสอบชื่อจังหวัดทีท่านกรอกอีกครั้ง"
)
setState({ ...state, value: "" })
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't setState clear the input box?

Copy link
Author

Choose a reason for hiding this comment

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

I try to use the existing value, but the error page is shown.

It seems like, if I set the value to the current (incorrect province's name) one, it tries to search non-exist province and show the following error:

image

So, for now, I clear it out to prevent the problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

yep. The function component renderFilter(filterName, label="") require filterName to be valid otherwise it will crash. Also, it goes back to the previous successful query due to onBlur (since we cannot leave renderFilter to be invalid).

Copy link
Contributor

Choose a reason for hiding this comment

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

but it seems strange since I've already used state.query not state.value...

@itpcc
Copy link
Author

itpcc commented Mar 25, 2019

I've added a prop called noOnBlur on the input box after the alert box is displayed. So, when onBlur is called, no further setState call and crash the app.

I know it's kinda "hack" approach, but I hope it's good enough.

@codeclimate
Copy link

codeclimate bot commented Mar 26, 2019

Code Climate has analyzed commit f8b5ad4 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 0.0% (0% is the threshold).

This pull request will bring the total coverage in the repository to 7.8% (-0.1% change).

View more on Code Climate.

@itpcc
Copy link
Author

itpcc commented Mar 26, 2019

I've changed the error display scheme from alert to <span> tag instead.

In this case, when the error occurs, it'll set state provinceError to true. Then, it'll change the input box's border color to red and display the error message below the input box. :

on mobile
image

on desktop
image

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.

3 participants