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

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

Closed
wants to merge 2 commits into from

Conversation

Yamaton0827
Copy link
Contributor

Issue

  • close #ISSUE_NUMBER

Overview (Required)

  • Empty view of the search screen is not aligned center vertically.
    In figma, UFO image and description is centered in whole screen (including app top bar and search filters bar).
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

Copy link

github-actions bot commented Sep 8, 2024

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

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 8, 2024 07:34 Inactive
Copy link

github-actions bot commented Sep 8, 2024

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

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 8, 2024 14:07 Inactive
@takahirom
Copy link
Member

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
Copy link
Contributor

naoele commented Sep 8, 2024

@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
Copy link
Member

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
Copy link
Contributor

naoele commented Sep 8, 2024

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
Copy link
Member

I think this is a somewhat difficult problem. We also have this issue, and this PR could complicate things further.
#520

@naoele
Copy link
Contributor

naoele commented Sep 9, 2024

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

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.

3 participants