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

Empty state for payloads #325

Merged
merged 6 commits into from
Apr 16, 2020
Merged

Empty state for payloads #325

merged 6 commits into from
Apr 16, 2020

Conversation

vbuberen
Copy link
Collaborator

📷 Screenshots

Request Response
Screenshot 2020-04-15 at 13 21 35 Screenshot 2020-04-15 at 13 21 58

📄 Context

This PR closes #276 by checking request or response payload and showing corresponding state in case any of them is empty.

📝 Changes

  • Added empty state view into layout for requests and responses.
  • Slightly changed the way TransactionPayloadAdater is declared and gets its items.
  • Moved adapter attachment to RecyclerView to onViewCreated, so we could get rid of logcat messages No adapter attached.

📎 Related PR

This PR would require some updates in #305 and #323, since there were changes in same classes.

⏱️ Next steps

While working on this PR I came to conclusion that we should move processPayload method to TransactionViewModel, because it is left from times when we had no ViewModels at all and because transformation of items into view items looks like a job for ViewModel.

However, I would postpone it till #305, #323 and my planned PR with search improvement gets merged to avoid too much updates requirements for those PRs.

@vbuberen vbuberen added the enhancement New feature or improvement to the library label Apr 15, 2020
@vbuberen vbuberen requested a review from cortinico April 15, 2020 10:35
// Invalidating menu, because we need to hide menu items for empty payloads
requireActivity().invalidateOptionsMenu()

payloadBinding.loadingProgress.visibility = View.GONE
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really sure what is going here. Why is this view first set to VISIBLE and then to GONE in the same lambda?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a ProgressBar. We show it when trying to fetch the payload and hide when it is fetched.

Copy link
Contributor

Choose a reason for hiding this comment

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

But is it suspended or something? Wouldn't it result always in GONE since the code is executed in one function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it works fine with showing progress on screen opening and hiding it properly.

Copy link
Contributor

@MiSikora MiSikora Apr 15, 2020

Choose a reason for hiding this comment

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

Ahh… I checked the code out in the IDE. Process payload suspends so it allows for changing progress visibility. Too bad it is not that obvious via GH. :(

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Generally looks good 👍 Just a couple of minor comments.
The UI looks awesome btw!

Co-Authored-By: Nicola Corti <corti.nico@gmail.com>
@vbuberen
Copy link
Collaborator Author

What about the idea of moving processPayload into a ViewModel?

@vbuberen vbuberen merged commit 272cd91 into develop Apr 16, 2020
@vbuberen vbuberen deleted the empty_payload branch April 16, 2020 07:22
@cortinico cortinico added this to the 3.3.0 milestone Apr 16, 2020
@cortinico
Copy link
Member

What about the idea of moving processPayload into a ViewModel?

Yeah definitely 👍 Are you going to take care of this?

@vbuberen
Copy link
Collaborator Author

Yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add state for empty screens
3 participants