DroidKaigi / conference-app-2024

The Official Conference App for DroidKaigi 2024
Apache License 2.0
458 stars 200 forks source link

Changed the display position of EmptySearchResultBody to the center vertically. #1004

Closed Yamaton0827 closed 2 months ago

Yamaton0827 commented 2 months ago

Issue

Overview (Required)

figma emulated device

Links

Screenshot (Optional if screenshot test is present or unrelated to UI)

Before After After: show layout bounds
mode ON

Movie (Optional)

Before After
github-actions[bot] commented 2 months ago

Detekt check failed. Please run ./gradlew detekt --auto-correct to fix the issues.

github-actions[bot] commented 2 months ago
Snapshot diff report File name Image
SearchScreenTest[Sea
rchScreen - when ser
ver is operational -
when filter categor
y chip click - when
click category App A
rchitecture en - it
should selected cate
gory App Architectur
e en]_compare.png
SearchScreenTest[Sea
rchScreen - when ser
ver is operational -
when filter day chi
p click - when click
conference day 2 -
it should selected d
ay 2]_compare.png
SearchScreenTest[Sea
rchScreen - when ser
ver is operational -
when filter languag
e chip click - it sh
ould show drop down
menu]_compare.png
SearchScreenTest[Sea
rchScreen - when dev
ice is tablet - it s
hould no timetable i
tems are displayed]_
compare.png
SearchScreenTest[Sea
rchScreen - when ser
ver is operational -
when filter categor
y chip click - when
click category Other
en - it should sele
cted category Other
en]_compare.png
SearchTextFieldAppBa
rPreview_FilledSearc
hWord_compare.png
SearchScreenTest[Sea
rchScreen - when ser
ver is operational -
when filter languag
e chip click - when
click language MIXED
- it should selecte
d language MIXED]_co
mpare.png
SearchScreenTest[Sea
rchScreen - when ser
ver is operational -
when filter categor
y chip click - it sh
ould show drop down
menu]_compare.png
SearchScreenTest[Sea
rchScreen - when ser
ver is operational -
when filter languag
e chip click - when
click language ENGLI
SH - it should selec
ted language ENGLISH
]_compare.png
EmptySearchResultBod
yPreview_compare.png
SearchScreenTest[Sea
rchScreen - when ser
ver is operational -
input search word t
o TextField - it sho
uld show search word
and filtered items]
_compare.png
SearchScreenTest[Sea
rchScreen - when ser
ver is operational -
when filter categor
y chip click - when
click category Jetpa
ck Compose en - it s
hould selected categ
ory Jetpack Compose
en]_compare.png
SearchScreenTest[Sea
rchScreen - when ser
ver is operational -
when filter day chi
p click - when click
conference day 1 -
it should selected d
ay 1]_compare.png
SearchScreenTest[Sea
rchScreen - when ser
ver is operational -
when filter day chi
p click - it should
show drop down menu]
_compare.png
SearchTextFieldAppBa
rPreview_EmptySearch
Word_compare.png
SearchScreenTest[Sea
rchScreen - when ser
ver is operational -
it should no timeta
ble items are displa
yed]_compare.png
SearchScreenTest[Sea
rchScreen - when dev
ice is tablet - inpu
t search word to Tex
tField - it should s
how search word and
filtered items]_comp
are.png
EmptySearchScreenPre
view_compare.png
SearchScreenTest[Sea
rchScreen - when ser
ver is operational -
when filter languag
e chip click - when
click language JAPAN
ESE - it should sele
cted language JAPANE
SE]_compare.png
takahirom commented 2 months ago

Thank you for this valuable contribution. I believe we need this implementation to comply with the design. The problem is that we need to modify the globalPositioned each time we change the structure of a composable or add a new composable. This could make our development process difficult. I'm considering whether it might be better not to proceed with this pull request. 🙇

naoele commented 2 months ago

@Yamaton0827 -san @takahirom -san Sorry to butt in, but this can be fixed with a much smaller change. Rather than changing globalPositioned, It would be better to get the size of the top element and padding it accordingly.

    var favoriteFiltersSize by remember { mutableStateOf(0.dp) }

    Column(modifier = modifier.fillMaxSize()) {
        FavoriteFilters(
            allFilterSelected = uiState.isAllFilterSelected,
            day1FilterSelected = uiState.isDay1FilterSelected,
            day2FilterSelected = uiState.isDay2FilterSelected,
            backgroundColor = filterBackgroundColor,
            onAllFilterChipClick = onAllFilterChipClick,
            onDay1FilterChipClick = onDay1FilterChipClick,
            onDay2FilterChipClick = onDay2FilterChipClick,
            modifier = Modifier
                .fillMaxWidth()
                .background(favoriteFiltersBackgroundColor)
                .onSizeChanged { size ->
                    favoriteFiltersSize = size.height.dp
                },
        )

        when (uiState) {
            is FavoritesSheetUiState.Empty -> {
                EmptyView(
                    modifier = Modifier.wrapContentSize(Alignment.Center)
                        .padding(bottom = WindowInsets.statusBars.asPaddingValues().calculateTopPadding() + favoriteFiltersSize)
                )
            }

Screenshot

It's a little bit off from Figma, but what do you think of this change?

Before After Figma

I don't mind if you use this code if you like.

By the way, it would be helpful if you could run detekt and organize the code.

takahirom commented 2 months ago

Thank you. That seems better, but I think it's not very good to add processing every time a component is added. However, if we handle it as an inset, I think it's relatively acceptable.

naoele commented 2 months ago

Sorry, I didn't understand what you meant by treating it as an inset. I suggested that this would be fine if no elements will be added to FavoritesScreen in the future.

takahirom commented 2 months ago

I think this is a somewhat difficult problem. We also have this issue, and this PR could complicate things further. https://github.com/DroidKaigi/conference-app-2024/issues/520

naoele commented 2 months ago

@takahirom -san, @Yamaton0827 -san I understand. If that's the situation, there's no need to consider my suggestion. Thank you for detailed explanation!

Yamaton0827 commented 2 months ago

@takahirom Thank you for your review and consideration. The content of the discussion was a little difficult for me, so I will try to understand it from now on.

@naoele Thank you for the suggestion, it was helpful.

Yamaton0827 commented 2 months ago

This pull request is closed because of a fundamental problem. Thank you for your review.