[Refactor] Remove multiple instances of Flutter toast widgets. #737
Conversation
| @@ -81,7 +78,9 @@ class RegisterFormState extends State<RegisterForm> { | |||
| setState(() { | |||
| _progressBarState = false; | |||
| }); | |||
| _exceptionToast(result.hasException.toString().substring(16, 35)); | |||
| Fluttertoast.showToast( | |||
| msg: result.hasException.toString().substring(16, 35), | |||
There was a problem hiding this comment.
What does the absolute values mean? can these be in a constant variable with descriptive names?
There was a problem hiding this comment.
For example resultsMessageStartPosition and resultsMessageEndPosition
There was a problem hiding this comment.
What does the absolute values mean? can these be in a constant variable with descriptive names?
@dellyjm by absolute values do you mean the integer values (16 and 35) ?
There was a problem hiding this comment.
Already have a separate issue for error handling.
There was a problem hiding this comment.
What does the absolute values mean? can these be in a constant variable with descriptive names?
@dellyjm by absolute values do you mean the integer values (16 and 35) ?
yes, so name them so that the next developer reviewing understands what the start and end values mean without googling.
There was a problem hiding this comment.
@dellyjm yes , the next developer may find it difficult to understand.
So the developer who coded this has also added a print statement to print the exception in the console.
Showing the whole exception to the user isn't a good idea.
So result.hasException.toString().substring(16, 35) displays only what the user may understand.
For eg:
This is printed in console (For devs):
ClientException: Failed to connect to http://talawa-graphql-api.herokuapp.com/graphql/talawa/graphql: Failed host lookup: 'talawa-graphql-api.herokuapp.com'
And a substring of that (from index 16 to 35) is for the user:
Failed to connect
These integer values may be different for different exceptions.
There is already a separate issue for error handling refer: issue #673
| @@ -68,7 +67,9 @@ class _UrlPageState extends State<UrlPage> | |||
| LogHelper().log(LogLevel.ERROR, widget.toStringShort(), "checkAndSetUrl", | |||
| "Incorrect Oraganization", | |||
| exception: e as Exception); | |||
| _exceptionToast('Incorrect Organization Entered'); | |||
| Fluttertoast.showToast( | |||
| msg: 'Incorrect Organization Entered', | |||
There was a problem hiding this comment.
error messages need to be abstracted out to internationalization file. multiple languages are supported.
There was a problem hiding this comment.
Hi @dellyjm, as per my knowledge multilingual support is not added yet, it's a part of the GSoC project.
Please see this link : GSoC ideas
There was a problem hiding this comment.
error messages need to be abstracted out to internationalization file. multiple languages are supported.
@dellyjm We'll open issues on internationalization next week. Like @utkarshshendge said, some GSoC applications may include this feature and it'll be best to wait and see whether any of these are accepted.
| @@ -140,7 +132,9 @@ class _AddEventState extends State<AddEvent> { | |||
| ); | |||
| print('Result is : $result'); | |||
| if (result == null) { | |||
| _exceptionToast("Could not create event! Please Try Again later!"); | |||
| Fluttertoast.showToast( | |||
| msg: "Could not create event! Please Try Again later!", | |||
There was a problem hiding this comment.
no hardcoding of strings. Internationalization file please.
There was a problem hiding this comment.
This is going to be already big a seperate issue for that would be much better is guess.
| } else if (result.hasException) { | ||
| print(result.exception); | ||
| setState(() { | ||
| _progressBarState = false; | ||
| }); | ||
|
|
||
| _exceptionToast(result.exception.toString().substring(16, 35)); | ||
| Fluttertoast.showToast( |
There was a problem hiding this comment.
Hi @utkarshshendge
Can you break the exception here like whether it is GraphqlError or it is ClientException and display message accordingly. It would enhance the experience of user as he/she gets to know whether it is a server side problem or something related to their phone connectivity.
There was a problem hiding this comment.
@sumitra19jha this is much needed. But it looks a bit out of the scope of this PR.
I think it will be best if a new issue is created for it so we can apply these changes in the entire App.
@dellyjm Should I create a separate issue for this or fix it as a part of this PR?
There was a problem hiding this comment.
@utkarshshendge if possible add the changes as part of this PR only. Update issue and PR description.
Also fix conflicts on this PR.
|
@utkarshshendge please fix the merge conflicting files |
|
@palisadoes happened a mistake while merging it closing the PR |
What kind of change does this PR introduce?
This is a code refactor.
This issue fixes #716 and
Coincidentally fixes #594 .
Did you add tests for your changes?
No, not required
Summary
Instead of creating separate widgets I directly used
FlutterToast.showTost(). It gives us the same behavior, prevents text-overflow, and significantly reduces the number of lines in code.Does this PR introduce a breaking change?
No