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

Update UI Display when no item or not connected #286

Merged
merged 8 commits into from
Jun 28, 2022

Conversation

yschimke
Copy link
Collaborator

@yschimke yschimke commented Jun 28, 2022

WHAT

image

image

WHY

Avoid blank space, as per UX designs.

HOW

DefaultPlayerScreenMediaDisplay includes logic to show other status information, while DefaultMediaDisplay shows the track details only.

Checklist 📋

  • Add explicit visibility modifier and explicit return types for public declarations
  • Run spotless check
  • Run tests
  • Update metalava's signature text files

@yschimke yschimke marked this pull request as draft June 28, 2022 15:29
@yschimke yschimke requested a review from luizgrp June 28, 2022 15:41
@yschimke yschimke marked this pull request as ready for review June 28, 2022 15:41
modifier: Modifier = Modifier
) {
val mediaItem = playerUiState.mediaItem
if (!playerUiState.connected) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: given the UI layer has its own model PlayerUiState, it could be changed to attend the specific needs of the UI classes, e.g. playerUiState.loading instead of playerUiState.connected; or playerUiState.mediaDisplayStatus be either of values Loading, Playing or NotPlaying.

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'll follow up on this and below as I'm building on this currently.

)
} else {
InfoMediaDisplay(
message = stringResource(R.string.horologist_nothing_playing),
Copy link
Member

Choose a reason for hiding this comment

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

nit: I see the benefit of the library providing default content descriptions to the icons it displays (which can be overridden to provide translations) but I feel that displaying a text on the screen in a specific language by default is more invasive and would rather require this as parameter to the component - I might be biased and maybe I should not see any difference in between these two scenarios

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'll follow up on this .

/**
* Button to launch a screen to control the system volume.
*
* See [VolumeState]
Copy link
Member

Choose a reason for hiding this comment

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

nit: update kdoc

mediaItem,
mediaItemPosition,
shuffleModeEnabled,
// TODO work out how to combine more than 5 flows
Copy link
Member

Choose a reason for hiding this comment

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

😱

combine(
  combine(f1,f2,f3,f4,f5, map),
  combine(f6,f7,f8,f9,f10, map),
  map
)

😅

@yschimke yschimke merged commit 4869bc7 into google:main Jun 28, 2022
@yschimke yschimke deleted the no_item branch July 11, 2022 10:05
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