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

Yim23/design fixes #329

Merged
merged 28 commits into from
Jan 10, 2024
Merged

Conversation

pranavkonidena
Copy link
Contributor

Don't Merge till #328 is merged

This PR incorporates design fixes as pointed out by aerozol here (https://docs.google.com/document/d/1XLu5srnP6j5ehW6CKtLPrhxmLWNpxG_eHeKV1W9K22k/edit)

@@ -103,10 +104,10 @@ fun Yim23HomeScreen(
Image(
painter =
painterResource(id = when (viewModel.themeType.value) {
0 -> R.drawable.yim23_pick_color_green
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the Yim23ThemeData (Renamed to Yim23ThemeType) enum class, create a parameter with name pickColorRes and use viewModel.themeType.pickColorRes everywhere instead of running a when clause every time.
add other fields as well like colorRes, logoRes etc.

1 -> R.drawable.yim23_pick_color_red
2 -> R.drawable.yim23_pick_color_blue
3 -> R.drawable.yim23_pick_color_grey
Yim23ThemeData.greenTheme -> R.drawable.yim23_pick_color_green
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename the enum class and member themes as such:

enum class Yim23ThemeType(@DrawableRes val colorRes: Int) {
  GREEN(pickColorRes = R.drawable.yim23_pick_color_green, colorRes = ...),
  RED(...),
  BLUE(...),
  GREY(...)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya thats so better.. will do that thx

@@ -30,7 +31,7 @@ import javax.inject.Inject
class Yim23ViewModel @Inject constructor(
private val repository: Yim23Repository,
private val appPreferences: AppPreferences
) : BaseYimViewModel() {
) : ViewModel() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use BaseViewModel here instead.

color = MaterialTheme.colorScheme.background ,
style = MaterialTheme.typography.labelLarge , maxLines = 1)
Yim23BaseScreen(
viewModel = viewModel,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this, but for parameters, 1 space separation should be followed.

@@ -38,6 +42,15 @@ class SocialViewModel @Inject constructor(
private val remotePlaybackHandler: RemotePlaybackHandler,
@IoDispatcher private val ioDispatcher: CoroutineDispatcher,
): FollowUnfollowModel<SocialUiState>(repository, ioDispatcher) {

var friendsData : MutableState<
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope delete this.

repository.getFollowers(username)
}
return@runBlocking deferredResult.await()
fun getFollowers() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need this function to be synchronous and have UI do state management on its own since this function won't be used everytime this View model is created.

Suggested change
fun getFollowers() {
suspend fun getFollowers(): Resource<SocialData> {
val username = appPreferences.getUsername()
return repository.getFollowers(username).also {
if(it.status == Resource.Status.FAILED){
emitError(result.error)
}
}
}

Some of the usages of appPreferences.getUsername() are old and use withContext(ioDispatcher) but the approach is actually redundant.

@07jasjeet 07jasjeet mentioned this pull request Jan 8, 2024
@07jasjeet
Copy link
Collaborator

@pranavkonidena Few Issues:

  1. I still see crashes when scrolling down a list.
  2. First screen is messed up for Devices with longer length.
  3. Music Buddies screen 1 has different enter/exit animation than rest.
  4. Both Music Buddies screens show weird loading signs that should not be there. Consider pre-loading followers and similar users in your Yim23ViewModel because your implementation demands so but you must not touch SocialViewModel because its a general use view-model.
  5. Top and Bottom bars are still a mess.

@@ -250,4 +250,7 @@ dependencies {
// Third party libraries
implementation 'com.github.a914-gowtham:compose-ratingbar:1.3.4'
implementation 'com.github.akshaaatt:Logger-Android:1.0.0'

//Charting Library (Vico)
implementation("com.patrykandpatrick.vico:compose:1.13.1")
Copy link
Member

Choose a reason for hiding this comment

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

Can we please give an attribution to this library on the settings page? I think we already attribute the animations and icons we use there

Copy link
Member

@akshaaatt akshaaatt left a comment

Choose a reason for hiding this comment

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

@pranavkonidena please never merge main into your PRs. Although it shouldn't do anything but you shouldn't be concerned about doing so

@pranavkonidena
Copy link
Contributor Author

@akshaaatt So sorry, meant to update this PR with latest dev branch

@pranavkonidena
Copy link
Contributor Author

Updated video for YIM23 ->
https://usercontent.irccloud-cdn.com/file/qtYqQMnu/IMG_7078.MP4

@07jasjeet
Copy link
Collaborator

More work has to be done but looks fine for beta.

@07jasjeet
Copy link
Collaborator

@akshaaatt your call if we should fix things here on in next PR.

Copy link
Member

@akshaaatt akshaaatt left a comment

Choose a reason for hiding this comment

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

LGTM for now. Let's fix things in the next PR

@akshaaatt akshaaatt merged commit 74d6078 into metabrainz:dev Jan 10, 2024
1 check passed
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