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.

Sign up for GitHub

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

Jump to bottom

Remove "interpretation target" chip and add another lang chip when the session is interpretation target #571

Merged
merged 3 commits into from
Aug 17, 2023
Merged

Remove "interpretation target" chip and add another lang chip when the session is interpretation target #571

merged 3 commits into from
Aug 17, 2023

Conversation

tfandkusu
Copy link
Contributor

@tfandkusu tfandkusu commented Aug 16, 2023 •
edited
Loading

Issue

Overview (Required)

In the TimetableListItem of TimetableList

  • When session is interpretation target, labels are EN JA
  • When session is English only, label is EN
  • When session is Japanese only, label is JA
  • When session is Mixed, label is MIXED. There is no Mixed session, but I adapted the existing implementation.

Links

Screenshot

Before After

@github-actions
Copy link

github-actions bot commented Aug 16, 2023 •
edited
Loading

Test Results

47 tests   47 ✔️  3m 41s ⏱️
  8 suites    0 💤
  8 files      0 ❌

Results for commit 1241ab8.

♻️ This comment has been updated with latest results.

@github-actions github-actions bot temporarily deployed to deploygate-distribution August 16, 2023 13:56 Inactive
@tfandkusu tfandkusu changed the title Remove "interpretation target" chip and align the chip to the Figma Remove "interpretation target" chip Aug 16, 2023
@tfandkusu tfandkusu marked this pull request as ready for review August 16, 2023 14:08
@takahirom
Copy link
Member

takahirom commented Aug 16, 2023 •
edited
Loading

Sorry, it says align the chip to the Figma, so can you add another lang when the session is interpretation target?
image

@tfandkusu tfandkusu changed the title Remove "interpretation target" chip Remove "interpretation target" chip and add another lang when the session is interpretation target? Aug 16, 2023
@tfandkusu tfandkusu changed the title Remove "interpretation target" chip and add another lang when the session is interpretation target? [WIP]Remove "interpretation target" chip and add another lang when the session is interpretation target? Aug 16, 2023
@tfandkusu tfandkusu changed the title [WIP]Remove "interpretation target" chip and add another lang when the session is interpretation target? [WIP]Remove "interpretation target" chip and add another lang when the session is interpretation target Aug 16, 2023
@tfandkusu tfandkusu changed the title [WIP]Remove "interpretation target" chip and add another lang when the session is interpretation target Remove "interpretation target" chip and add another lang when the session is interpretation target Aug 16, 2023
@tfandkusu tfandkusu changed the title Remove "interpretation target" chip and add another lang when the session is interpretation target Remove "interpretation target" chip and add another lang chip when the session is interpretation target Aug 16, 2023
@github-actions github-actions bot temporarily deployed to deploygate-distribution August 16, 2023 16:30 Inactive
@tfandkusu
Copy link
Contributor Author

I updated Overview, Screenshot and implementation.

@mhidaka
Copy link
Member

mhidaka commented Aug 16, 2023

Brilliant! Thank you for your quick commits! We'll take a p-r review time tomorrow. 💪💪💪

takahirom
takahirom reviewed Aug 17, 2023
View reviewed changes
core/model/src/commonMain/kotlin/io/github/droidkaigi/confsched2023/model/TimetableLanguage.kt
else -> {
langOfSpeaker.take(2)
}
val labels = if (langOfSpeaker == Lang.MIXED.tagName) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks great

takahirom
takahirom approved these changes Aug 17, 2023
View reviewed changes
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.

Thanks for your cool code! 🆒

@takahirom takahirom merged commit 404cca1 into DroidKaigi:main Aug 17, 2023
5 checks passed
@tfandkusu tfandkusu deleted the issue_463 branch August 17, 2023 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@takahirom takahirom takahirom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove "interpretation target" chip and align the chip to the Figma
3 participants
@tfandkusu @takahirom @mhidaka