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

Add staff list page #557

Merged
merged 4 commits into from
Jan 20, 2019
Merged

Add staff list page #557

merged 4 commits into from
Jan 20, 2019

Conversation

mkano9
Copy link
Contributor

@mkano9 mkano9 commented Jan 20, 2019

Issue

Overview (Required)

  • Added staff list page

Screenshot

|
device-2019-01-20-013427

@jmatsu-bot
Copy link
Collaborator

@takahirom
Copy link
Member

🆒

android:layout_marginBottom="8dp"
android:layout_marginStart="16dp"
android:layout_marginTop="8dp"
android:contentDescription="aaa"
Copy link
Collaborator

@jmatsu-bot jmatsu-bot Jan 20, 2019

Choose a reason for hiding this comment

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

⚠️ Hardcoded string “aaa”, should use @string resource

@takahirom
Copy link
Member

Sorry, we have to focus on developing the function of the session questionnaire so it will be a little late. 🙇
#556

@takahirom
Copy link
Member

The search function is very cool🆒

withContext(coroutineContext) {
database.runInTransaction {
val staffs = apiResponse.staffs.toStaffEntities()
staffDao.insert(staffs)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should use "deleteAll()" before insert(). Because If someone quits the staff for some reason, it seems to remain. 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good catch 💯

import io.github.droidkaigi.confsched2019.staff.databinding.ItemStaffBinding
import jp.wasabeef.picasso.transformations.CropCircleTransformation

class StaffItem constructor(
Copy link
Member

Choose a reason for hiding this comment

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

You can delete constructor 🏗

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ops

@takahirom
Copy link
Member

Almost LGTM 👍
Please fix danger warnings 🙏

@jmatsu-bot
Copy link
Collaborator

@jmatsu-bot
Copy link
Collaborator

@jmatsu-bot
Copy link
Collaborator

@jmatsu-bot
Copy link
Collaborator

Apk comparision results

Property Summary
New File Size 13925688 Bytes. (13.28 MB)
File Size Change -10240 Bytes. (-10.0 KB)
Download Size Change -12914 Bytes. (-12.61 KB)
New Method Reference Count 155074
Method Reference Count Change -370
New Number of dex file(s) 4
Number of dex file(s) Change 0

Generated by 🚫 Danger

@jmatsu-bot
Copy link
Collaborator

Asserted successfully. 💯

Generated by 🚫 Danger

Copy link
Member

@takahirom takahirom left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Thank you for a significant contribution. 🏆

@takahirom takahirom merged commit f1a8fc0 into DroidKaigi:master Jan 20, 2019
@mkano9
Copy link
Contributor Author

mkano9 commented Jan 20, 2019

@takahirom Thanks a lot for your support and for the opportunity to contribute :)

@takahirom takahirom mentioned this pull request Jan 23, 2019
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.

Add staff list screen
3 participants