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

Move time ago and score to a flowrow layout #1615

Merged
merged 9 commits into from
Aug 13, 2024
Merged

Conversation

dessalines
Copy link
Member

@dessalines dessalines commented Aug 7, 2024

A few reasons for doing this:

  • The SpaceBetween / SpaceAround layouts aren't going to work well when we start adding larging screen size support.
  • Reading through material design docs, the title should always be on top, with attribution and extra subtitles below.
  • Made the community icon on post listings smaller to look better.

What it looks like now:

Screenshot_2024-08-06-21-35-57-01_c9298fda48802cf3e2d17cb10a48759f

Screenshot_2024-08-06-21-16-33-78_c9298fda48802cf3e2d17cb10a48759f

Comes after #1614

- Adding a fade to the post body preview.
- Getting rid of muted, using colorScheme.outlined instead.
- Making label-type text use label rather than body style.
- Getting rid of some unecessary surface and onSurface colors.
- Fixes #1608
- Also separating time from score component.
- Using a start-based flowrow, for better responsiveness.
- Re-organizing PostCard / PostListingCard
@dessalines
Copy link
Member Author

I've been using this for a few days, and I'm liking it a lot.

@dessalines dessalines marked this pull request as ready for review August 9, 2024 17:35
@dessalines dessalines requested a review from MV-GH as a code owner August 9, 2024 17:35
Copy link
Member Author

@dessalines dessalines Aug 9, 2024

Choose a reason for hiding this comment

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

I split the card and list into different files, and this is now the generic one.

@@ -286,3 +286,28 @@ fun PostReply(
)
}
}

@Composable
fun PostNodeHeader(
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this as its only used here.

isDistinguished = isDistinguished,
isCommunityBanned = isCommunityBanned,
showAvatar = showAvatar,
val centerMod = Modifier.align(Alignment.CenterVertically)
Copy link
Member Author

@dessalines dessalines Aug 9, 2024

Choose a reason for hiding this comment

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

This centerMod is necessary on FlowRows unfortunately. I wish it could do some cross-axis alignment, but it doesn't seem to work.

fullBody = false,
)
}

// Title + metadata
PostBody(
post = postView.post,
read = postView.read,
postView = postView,
Copy link
Collaborator

Choose a reason for hiding this comment

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

These were all "optimizations"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any field that changes in postView will cause it recompose instead only if the relevant field changed

Copy link
Member Author

Choose a reason for hiding this comment

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

Erps, my bad, I'll go through and fix some of these.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I'm not sure if its worth it. A lot of these use like 5+ fields from PostView, and if that's the case, it just seems like we should pass in the full thing anyway.

IE, is there any field on PostView that changes frequently enough to justify separating these out? I don't think there is, as instantScores is the main one, but that's separated.

@MV-GH
Copy link
Collaborator

MV-GH commented Aug 9, 2024

I am honestly not such a fan of it. Maybe its worth here using a post view mode for the old view.

With current layout, it "forces" you to read community + votes each time. If you go like vertical: title -> content, you see the community + votes then the content.

it also seems to be behaving weirdly? With the community + user at the bottom? Or is that intended because I don't like that at all

image

vs old

image

Much better at using the "screen real estate", takes up less vertical space

Can directly see title -> content, want to see, user/community votes look up

@dessalines
Copy link
Member Author

dessalines commented Aug 9, 2024

The community line below the image post is a weirdness I couldn't figure out a great way around.

Part of the problem is that the title block wraps the post subtitle in a single row, but the community info is below that block. You can test this by looking at any link-type post (or going to small card mode to force images into a smaller thumbnail). Or look above, see how the bbc.com is to the left of the thumbnail, not below it like the community attribution line?

I could move the community info into that title block, or move the post subtitle below that title block. Both options aren't ideal, because they end up with wasted screen real estate, but I'll mess around with them.

A few possibilities I could use your input on:

  • I'm open to moving the community line above the post subtitle, or moving the subtitle out of that title block.
  • I'm open to any variation of ordering the hostname, vote score, and time ago. As long as its a left-starting flow layout.
  • I'm open to removing the hostname, although I do think its pretty useful to see that.

The main 2 things I want to keep are:

  • The smaller community / attribution line, below the title (as per material design standards)
  • Using left-aligned flowrows.

@dessalines dessalines marked this pull request as draft August 9, 2024 21:23
@MV-GH
Copy link
Collaborator

MV-GH commented Aug 9, 2024

The smaller community / attribution line, below the title (as per material design standards)

Is that really? If i look up sample M3 apps, they don't follow that at all.

image

Title -> content. Meta data around it

@MV-GH
Copy link
Collaborator

MV-GH commented Aug 9, 2024

moved

@dessalines
Copy link
Member Author

Title -> content. Meta data around it

Good point. I'll also try to go back to replicating Boost a little:

Screenshot_2024-08-09-18-07-42-90_044f1e467fa7f6fc20a65e6d6e356754

Could you copy-paste your PostViewMode comment to a new issue? I have some thoughts on that but it'd be better to put them on its own issue.

@dessalines
Copy link
Member Author

Does this look okay?

Screenshot_2024-08-09-19-04-27-73_c9298fda48802cf3e2d17cb10a48759f

@MV-GH
Copy link
Collaborator

MV-GH commented Aug 9, 2024

This looks better.

But personally I would like title directly against content. Have time top right and votes in bottom bar?

@dessalines
Copy link
Member Author

I'll give that a try.

@dessalines
Copy link
Member Author

dessalines commented Aug 10, 2024

Okay, I've moved the time into the header, and the score and voting back into the action bar. Here are some examples:

Showing full info (not the default setting obvi):

Screenshot_2024-08-10-15-29-45-04_c9298fda48802cf3e2d17cb10a48759f

Some voting examples:

Screenshot_2024-08-10-15-31-24-02_c9298fda48802cf3e2d17cb10a48759f

Screenshot_2024-08-10-15-31-53-77_c9298fda48802cf3e2d17cb10a48759f

- Moving time into the header bar, right aligned.
- Moving scores back to the action bar.
@dessalines dessalines marked this pull request as ready for review August 10, 2024 20:14
@dessalines
Copy link
Member Author

K this is ready to be looked at again.

@MV-GH
Copy link
Collaborator

MV-GH commented Aug 10, 2024

Great stuff

I believe we show the full title of the community? Maybe its better to just use the shorter "name"? And use the title in the opened post, where we have more space to work with?

image

Interesting behaviour

image

image

If its easier for you, it would definitely be okay with time being part of the flow row there. And it thus not always being in the top right.

@dessalines
Copy link
Member Author

dessalines commented Aug 10, 2024

It uses the shorter name only if its local. In lemmy-ui (and I think here?) we used to conditionally not show the @instance.tld based on various circumstances, but it just got too confusing and unpredictable. ppl like to instantly know if its a federated community or user.

I tried messing around with a few non-flow layouts there, but it always ends up looking strange. So ya I'd say lets just go with this for now.

If you'd like to mess around with it, its PostListingCard.kt L669 and L675

@MV-GH
Copy link
Collaborator

MV-GH commented Aug 10, 2024

It uses the shorter name only if its local.

With that I meant using name instead of title

Yeah sure. I have been testing this out and images sometimes "jump". Have you noticed that?

@dessalines
Copy link
Member Author

dessalines commented Aug 10, 2024

With that I meant using name instead of title

Hrm... we should pry just always prefer the display name if it exists, and I'd rather not more context-dependent rules around it. The reflow stuff is only an issue for people using the display name improperly, as a long description.

I have been testing this out and images sometimes "jump". Have you noticed that?

Yup... I didn't change anything related to images, and I'm convinced its a bug from one of the recent coil dep updates.

@dessalines dessalines requested a review from MV-GH August 13, 2024 17:54
@dessalines
Copy link
Member Author

dessalines commented Aug 13, 2024

@MV-GH lmk when you can get to reviewing these 2 PRs, I'd like to do a release this week, as I'll be away from a computer next week.

@MV-GH
Copy link
Collaborator

MV-GH commented Aug 13, 2024

Yeah sure.

Copy link
Collaborator

@MV-GH MV-GH left a comment

Choose a reason for hiding this comment

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

I haven't really tested. But I ll run master after this locally and see if anything pops up.

@dessalines
Copy link
Member Author

Cool.

@dessalines dessalines merged commit b6269b3 into main Aug 13, 2024
2 checks 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.

2 participants