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 share button to the Session Detail Screen #578

Closed
wants to merge 13 commits into from

Conversation

rkowase
Copy link
Collaborator

@rkowase rkowase commented Feb 3, 2018

Issue

Overview (Required)

  • Add share button to the Session Detail Screen

Links

Screenshot

Before After

@rkowase rkowase changed the title [WIP] Add share button to the Session Detail Screen Add share button to the Session Detail Screen Feb 3, 2018
session.room.name,
speakers)
val url = resources.getString(R.string.session_share_url, sessionId)
putExtra(Intent.EXTRA_TEXT, "$title\n$url")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

example

ConstraintLayout, now and future at Room 6 by Nicolas Roard, John Hoford, thagikura
https://droidkaigi.jp/2018/timetable?session=16988

Copy link
Collaborator Author

@rkowase rkowase Feb 3, 2018

Choose a reason for hiding this comment

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

[Q] Should I add a hashtag or any text?

Copy link
Member

Choose a reason for hiding this comment

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

Please wait. I will talk with staff🙏

Copy link
Member

Choose a reason for hiding this comment

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

The staff has decided to #droidkaigi_room1...#droidkaigi_room7 #droidkaigi_hall.

Copy link
Member

Choose a reason for hiding this comment

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

🙏

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added

e.g.

ConstraintLayout, now and future (Nicolas Roard, John Hoford, thagikura) #droidkaigi_room6
https://droidkaigi.jp/2018/timetable/?session=16988

*
* e.g. #droidkaigi_hall, #droidkaigi_room1, ...
*
* @param roomName The Room name

Choose a reason for hiding this comment

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

⚠️ Unnecessary space(s)

* e.g. #droidkaigi_hall, #droidkaigi_room1, ...
*
* @param roomName The Room name
* @return A hash tag

Choose a reason for hiding this comment

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

⚠️ Unnecessary space(s)

@@ -77,13 +78,22 @@ class SessionDetailFragment : Fragment(), Injectable {
binding.toolbar.setNavigationOnClickListener { activity?.finish() }
}

private fun bindSession(session: Session.SpeechSession) {
private fun bindSession(session: Session.SpeechSession, sessionId: String) {
Copy link
Member

Choose a reason for hiding this comment

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

I think you can get sessionId by session.id.
What do you think? 👀

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, I'll fix

@takahirom
Copy link
Member

takahirom commented Feb 4, 2018

I think two FABs are bad. Because FAB should be primary action. 👀
https://material.io/guidelines/components/buttons-floating-action-button.html

I think we should use menu icon.
What do you think?

private fun generateSharedText(session: Session.SpeechSession, sessionId: String): String {
var speakerList = listOf<String>()
session.speakers.forEach { speakerList += it.name }
val speakers = speakerList.joinToString(", ")
Copy link
Member

Choose a reason for hiding this comment

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

How about this? 👀
session.speakers.map { it.name }.joinToString(", ")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for suggestion. I'll simplify.

@@ -37,6 +37,11 @@
<string name="session_finished">Finished Session</string>
<string name="session_go_to_feedback">Go To Feedback</string>
<string name="session_current_badge" translatable="false">NOW!</string>
<string name="session_share_url" translatable="false">https://droidkaigi.jp/2018/timetable?session=%s</string>
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for translatable="false" 👍

@rkowase
Copy link
Collaborator Author

rkowase commented Feb 6, 2018

I think two FABs are bad. Because FAB should be primary action. 👀
https://material.io/guidelines/components/buttons-floating-action-button.html
I think we should use menu icon.

I agree. I'll use menu icon instead of FAB.

@rkowase
Copy link
Collaborator Author

rkowase commented Feb 7, 2018

I replaced.

Screenshot

Before After

@rkowase rkowase closed this Mar 10, 2021
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.

[Assigned] Add share button to the Session Detail Screen
4 participants