Skip to content

Mobile view passage overview rework - #738

Open
jeehoh wants to merge 36 commits into
developfrom
passage-overview-rework
Open

jeehoh wants to merge 36 commits into
developfrom
passage-overview-rework

Conversation

@jeehoh

@jeehoh jeehoh commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Major passage overview rework to match the design

Screenshots:

[Before]
before

[After]
after

jeehoh added 30 commits October 6, 2026 10:54
@jeehoh
jeehoh marked this pull request as ready for review October 12, 2026 19:34
@jeehoh
jeehoh requested a review from josephmyers October 12, 2026 19:35
@josephmyers

Copy link
Copy Markdown
Collaborator

Couple minor things:

  • Can we stack the Description directly under the Title. As it is, the Description appears to be at a set y-coord, accounting for two-line titles, which of course means for one-line titles there's an extra gap between them and the Description.
  • None of this was your change, but I think the "assignment" display is quite busy. Do we need to display both the avatar and the full username? I am interested to see the avatar only and also interested to see replacing the avatar with a grayscale person icon, as in the design. If you're okay experimenting...
  • Can we combine the Note's speech bubble icon into/as the graphic when there is no graphic chosen? I think showing both could be improved. It's just an idea; otherwise, I'd remove the speech bubble.
  • If there is no assignment, can we not show anything? Removing "Unassigned" will help with busyness.
  • As in the screenshot below, the card titles don't work well in Flat hierarchy. Should be an easy fix.
image

I'll start looking at the code itself now.

</Typography>
</Box>
<Box sx={{ display: 'flex', flexDirection: 'column', gap: 1.5 }}>
{!isPersonal && <PassageAssignee assign={cardInfo.assign} />}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah, I would not show PassageAssignee at all, unless there's an assignee.

>
{cardInfo.step}
</Button>
{playButton}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The Play button is centered with the card content and not the whole card. This makes the button look offcenter the smaller the card height is. It's minor, but noticeable


export function PassagePlayButton({
mediaId,
playing,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wasn't able to determine why playing needs to be passed in, i.e. controlled externally. I didn't see a good reason from the parents where this was needed. Ideally, the Play button would be able to play/pause its own audio, since the media is loaded here, as well.

flexShrink: 0,
}}
>
<CircularProgress

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good job getting this working and looking good

<PassagePlayButton
mediaId={mediaId}
playing={isPlaying}
active={isPlayActive}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we say "Enabled" instead of "Active"?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Actually, I may take this back. It's possible I don't fully understand this flag. Is it just resetting the UI when the play stops? It seems like there might be a way to do that without "needing to be told" externally

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