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

[Proposal] ♻️ Added the ability to go to the floor map from the session details. #1127

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Corvus400
Copy link
Contributor

Issue

  • None.

Overview (Required)

  • Added the ability to go to the floor map from the session details.
  • I really wanted to have the display switch between the first floor and the basement according to the floor level of each room, but I gave up on that this time.

Movie (Optional)

Before After
after.mp4

@Corvus400 Corvus400 requested a review from a team as a code owner September 8, 2023 19:05
Comment on lines 31 to 42
class DefaultNavigationRequester : NavigationRequester {
private val _navigateRequestStateFlow = MutableStateFlow("")
override fun getNavigationRouteFlow(): Flow<String> = _navigateRequestStateFlow

override fun navigateTo(route: String) {
_navigateRequestStateFlow.value = route
}

override fun navigated() {
_navigateRequestStateFlow.value = ""
}
}
Copy link
Contributor Author

@Corvus400 Corvus400 Sep 8, 2023

Choose a reason for hiding this comment

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

We did this to operate between different NavHosts.
I don't think this is the best way to do it, so if you have a better idea, I'm open to it.
I doubt if the name NavigationRequester is appropriate either.

@github-actions
Copy link

github-actions bot commented Sep 8, 2023

Hi @Corvus400! Codes seem to be unformatted. To resolve this issue, please run ./gradlew detekt --auto-correct and fix the results of ./gradlew lintDebug.. Thank you for your contribution.

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 8, 2023 20:08 Inactive
@github-actions
Copy link

github-actions bot commented Sep 8, 2023

Test Results

217 tests   217 ✔️  6m 52s ⏱️
  11 suites      0 💤
  11 files        0

Results for commit ad6726d.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Sep 8, 2023

Snapshot diff report

File name Image
io.github.droidkaigi
.confsched2023.sessi
ons.component_null_T
heme_CollapsedTimeta
bleTabRowPreview-Lig
htMode-26_26_null_co
mpare.png
io.github.droidkaigi
.confsched2023.sessi
ons.component_null_T
heme_TimetableTabPre
view-DarkMode-24_25_
null_compare.png
io.github.droidkaigi
.confsched2023.sessi
ons.component_null_T
heme_TimetableListIt
emPreview-LightMode-
22_22_null_compare.p
ng
KaigiAppTest.checkNa
vigateToTimetableIte
mDetailShot_compare.
png
io.github.droidkaigi
.confsched2023.sessi
ons.component_null_T
heme_TimetableListIt
emPreview-DarkMode-2
2_23_null_compare.pn
g
io.github.droidkaigi
.confsched2023.sessi
ons.section_null_The
me_TimetablePreview-
LightMode-29_29_null
_compare.png
io.github.droidkaigi
.confsched2023.sessi
ons.component_null_T
heme_TimetableTopAre
aPreview-LightMode-2
7_27_null_compare.pn
g
io.github.droidkaigi
.confsched2023.sessi
ons.section_null_The
me_SearchFilterPrevi
ew-LightMode-28_28_n
ull_compare.png
io.github.droidkaigi
.confsched2023.sessi
ons.component_null_T
heme_TimetableShimme
rListItemPreview-Dar
kMode-23_24_null_com
pare.png
io.github.droidkaigi
.confsched2023.sessi
ons.component_null_T
heme_TimetableItemDe
tailSummaryCardRowRo
omPreview-LightMode-
21_21_null_compare.p
ng
io.github.droidkaigi
.confsched2023.sessi
ons.component_null_T
heme_TimetableShimme
rListItemPreview-Lig
htMode-23_23_null_co
mpare.png
io.github.droidkaigi
.confsched2023.sessi
ons.component_null_T
heme_CollapsedTimeta
bleTabRowPreview-Dar
kMode-26_27_null_com
pare.png
io.github.droidkaigi
.confsched2023.sessi
ons.component_null_T
heme_TimetableItemDe
tailSummaryCardRowRo
omPreview-DarkMode-2
1_22_null_compare.pn
g
io.github.droidkaigi
.confsched2023.sessi
ons.component_null_T
heme_TimetableTabRow
Preview-LightMode-25
_25_null_compare.png
io.github.droidkaigi
.confsched2023.sessi
ons.component_null_T
heme_TimetableTabPre
view-LightMode-24_24
_null_compare.png
io.github.droidkaigi
.confsched2023.sessi
ons.section_null_The
me_TimetablePreview-
DarkMode-29_30_null_
compare.png
io.github.droidkaigi
.confsched2023.sessi
ons.section_null_The
me_SearchFilterPrevi
ew-DarkMode-28_29_nu
ll_compare.png
io.github.droidkaigi
.confsched2023.sessi
ons.component_null_T
heme_TimetableTabRow
Preview-DarkMode-25_
26_null_compare.png
io.github.droidkaigi
.confsched2023.sessi
ons.component_null_T
heme_TimetableTopAre
aPreview-DarkMode-27
_28_null_compare.png
TimetableItemDetailS
creenTest.checkSmall
FontScaleShot_compar
e.png
TimetableItemDetailS
creenTest.checkHugeF
ontScaleShot_compare
.png
TimetableItemDetailS
creenTest.checkLaunc
hShot_compare.png
TimetableItemDetailS
creenTest.checkLarge
FontScaleShot_compar
e.png
TimetableItemDetailS
creenTest.checkScrol
lShot_compare.png
TimetableItemDetailS
creenTest.checkBookm
arkToggleShot_compar
e.png

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 8, 2023 20:51 Inactive
@@ -109,6 +112,10 @@ fun TimetableItemDetailScreen(
onNavigationIconClick = onNavigationIconClick,
onBookmarkClick = viewModel::onBookmarkClick,
onLinkClick = onLinkClick,
onRoomClick = {
viewModel.navigateTo("floorMap")
Copy link
Contributor Author

@Corvus400 Corvus400 Sep 8, 2023

Choose a reason for hiding this comment

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

I would like to change this part if possible since it is directly specified.
It would be helpful if FloorMapScreenRoute could be referenced in this case.

@@ -111,6 +111,9 @@ private fun KaigiNavHost(
onTimetableItemClick = navController::navigateToTimetableItemDetailScreen,
onNavigateToBookmarkScreenRequested = navController::navigateToBookmarkScreen,
onLinkClick = externalNavController::navigate,
onRoomClick = {
navController.popBackStack(navController.graph.startDestinationId, false)
Copy link
Member

@takahirom takahirom Sep 9, 2023

Choose a reason for hiding this comment

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

Well, it's difficult to think but what if we create navController::navigateToMainFloorMapScreen?
I think we can see parent navController to access getBackStackEntry 👀

Copy link
Contributor Author

@Corvus400 Corvus400 Sep 9, 2023

Choose a reason for hiding this comment

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

Once I was able to return to the start destination with the following code.

fun NavController.navigateToMainFloorMapScreen() {
    getBackStackEntry(timetableItemDetailScreenRoute).destination.parent?.startDestinationId?.let {
        navigate(it)
        // TODO navigateMainFloorMap
    }
}

However, I have not figured out how to move to FloorMapScreen from here without having to make any major changes.
Do you already have any ideas? 🙇

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this. But I think we can pass the route parameter to NavHost and we may be able to pass /main/floorMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@takahirom
I have tried the suggested method, but unfortunately the app crashes. 🙇
We still think that as long as NavHost is split into two, it is unfortunately difficult to achieve this functionality other than this current implementation. 🙇

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 10, 2023 03:27 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 10, 2023 16:36 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 11, 2023 16:31 Inactive
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.

2 participants