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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Showed progress bar on sessions page #125

Merged
merged 10 commits into from Jan 14, 2018

Conversation

Projects
None yet
3 participants
@katsutomu
Contributor

katsutomu commented Jan 13, 2018

Issue

Overview (Required)

  • Show loading view on sessions page
  • Defined common progressbar style
    • need it? 馃檱

Links

  • none

Screenshot

device

@takahirom

This comment has been minimized.

Member

takahirom commented Jan 13, 2018

馃憖

@konifar

This comment has been minimized.

Contributor

konifar commented Jan 13, 2018

Looks good! Can you put the screenshot just in case? 馃檱

@@ -36,5 +36,8 @@
<item name="android:fitsSystemWindows">true</item>
</style>
<style name="ProgressBar" parent="android:Widget.ProgressBar.Large">

This comment has been minimized.

@takahirom

takahirom Jan 13, 2018

Member

We should use Widget.AppCompat.ProgressBar for showing modern progress bar 鈾伙笍

This comment has been minimized.

@katsutomu

katsutomu Jan 13, 2018

Contributor

馃檱

@katsutomu

This comment has been minimized.

Contributor

katsutomu commented Jan 13, 2018

Sorry... The theme was not reflected in this.
Please wait a moment as I fix it.

@katsutomu

This comment has been minimized.

Contributor

katsutomu commented Jan 13, 2018

@takahirom
Fixed to display modern progress bar.
Please re-review 馃檱

@takahirom

This comment has been minimized.

Member

takahirom commented Jan 13, 2018

In my environment, when switching to the sessions on the bottom bar, I saw two progress bars. 馃
Does not the same thing happen in your environment?

@katsutomu

This comment has been minimized.

Contributor

katsutomu commented Jan 13, 2018

oh... I see.
Progress bar in SessionsFragment is displayed first, progress bar in AllsessionsFragment displayed next.
Depending on the timing, you may see two progressbar.

Change the display timing?

@takahirom

This comment has been minimized.

Member

takahirom commented Jan 13, 2018

If you have time, Please use this class and normal ProgressBar. And can you adjust delayMs? 馃檹
https://twitter.com/chrisbanes/status/927928428539580416

@katsutomu

This comment has been minimized.

Contributor

katsutomu commented Jan 13, 2018

Cool! I try.
Define the class in io.github.droidkaigi.confsched2018.util, is it okay?
and I will adjust the delayMs by measuring it.

@takahirom

This comment has been minimized.

Member

takahirom commented Jan 13, 2018

OK 馃憤

sessionsViewModel.rooms.observe(this, { result ->
when (result) {
is Result.InProgress -> {
binding.progress.show()
}

This comment has been minimized.

@katsutomu

katsutomu Jan 13, 2018

Contributor

With this approach, two progress bar at reopen from background. 鈾伙笍

@katsutomu

This comment has been minimized.

Contributor

katsutomu commented Jan 13, 2018

Fixed. prease re-review!
Takes time to load sessions was less than about 500 ms.
and I also updated screenshot.

@takahirom

This comment has been minimized.

Member

takahirom commented Jan 13, 2018

I will check out and check it!馃憤

@@ -0,0 +1,52 @@
package io.github.droidkaigi.confsched2018.util

This comment has been minimized.

@takahirom
@takahirom

This comment has been minimized.

Member

takahirom commented Jan 14, 2018

It works well! 馃憤
Do you know why showing two progress bars when using Result.InProgress?馃

katsutomu added some commits Jan 14, 2018

@katsutomu

This comment has been minimized.

Contributor

katsutomu commented Jan 14, 2018

Fixed review! Prease re-review 馃檱

Do you know why showing two progress bars when using Result.InProgress?馃

It has been re-loding data at reopen from background. In the case of use ContentLoadingProgressBar, It was not displayed because it was calling hide().

@takahirom

This comment has been minimized.

Member

takahirom commented Jan 14, 2018

Thank you 馃憤

@takahirom takahirom merged commit 60ffb59 into DroidKaigi:master Jan 14, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
danger/danger 鈿狅笍 27 Warnings. Don't worry, everything is fixable.
@katsutomu

This comment has been minimized.

Contributor

katsutomu commented Jan 14, 2018

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment