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

feat: Add retry button in case of Internet Error #1105

Merged
merged 6 commits into from
Feb 11, 2022
Merged

feat: Add retry button in case of Internet Error #1105

merged 6 commits into from
Feb 11, 2022

Conversation

danagbemava
Copy link
Contributor

@danagbemava danagbemava commented Feb 11, 2022

What

  • This PR adds a retry button to the SmoothProductCardError widget.

Video

telegram-cloud-document-4-5780477560336944279.mp4

Fixes bug(s)

Part of

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Looks good to me @danagbemava!

Several remarks though:

  • perhaps renaming your PR "feat: " instead of "[feat] " would be more git-effective - to be confirmed by @M123-dev
  • while you're here, you could make setBarcodeState private (as _setBarcodeState)
  • I believe you should create branches instead of coding directly on develop. But I'm not an expert on git. Really not.
  • The icing on the cake would be to check the network availability - I'm not even talking of callbacks when the connection is up again - like before suggesting a refresh to the end-user, check the network first. Not in this PR. We can see later if we need to spend time on this feature.

Copy link
Member

@M123-dev M123-dev left a comment

Choose a reason for hiding this comment

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

Thanks a lot @danagbemava, the code looks good.

To what @monsieurtanuki said, yes we follow the convention for naming feat: / fix: / ... so that they get picked up by a intigration which automatically creates the changelog (#962) , but that's just a minor detail.

@danagbemava danagbemava changed the title [feat] Add retry button in case of Internet Error feat: Add retry button in case of Internet Error Feb 11, 2022
@danagbemava danagbemava requested a review from a team as a code owner February 11, 2022 19:10
@danagbemava
Copy link
Contributor Author

Thanks for the feedback.

while you're here, you could make setBarcodeState private (as _setBarcodeState)

I believe you should create branches instead of coding directly on develop

I had considered this, but I didn't envision this PR to touch a lot of files in my initial thought process, but I'll keep that in mind for future contributions.

The icing on the cake would be to check the network availability - I'm not even talking of callbacks when the connection is up again - like before suggesting a refresh to the end-user, check the network first. Not in this PR. We can see later if we need to spend time on this feature.

I considered something similar, where I would check the network state and show an error Snackbar in case the network wasn't available and make the request if it was available but I was just finding my way around the codebase (still exploring it now) and I wanted to get my feet wet. I'll keep that in mind as something to implement in the future.

@M123-dev M123-dev merged commit a1e8221 into openfoodfacts:develop Feb 11, 2022
@M123-dev
Copy link
Member

Thanks @danagbemava I've just merged your pull request, it is now integrated to the develop branch 🥳

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