Skip to content

Conversation

@oliverguenther
Copy link
Member

@oliverguenther oliverguenther force-pushed the feat/meetings-presentation-mode branch 2 times, most recently from b00b480 to ff9c59d Compare October 30, 2025 14:02
@mrmir mrmir self-requested a review October 30, 2025 14:11
@opf opf deleted a comment from github-actions bot Oct 30, 2025
Copy link
Contributor

@mrmir mrmir left a comment

Choose a reason for hiding this comment

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

Great job! I really like it 👏🏽

I noticed some things:

  1. Presenting an empty meeting needs to either not be possible or handled more gracefully. It returns a NoMethodError right now
  2. While editing an agenda item in presentation mode, if it is updated somewhere else, the current edit state is lost and the component is reloaded with the externally updated content. The reload shouldn't be happening at all when an item is being edited according to the AC
  3. The UI doesn't exactly match the specs (presentation mode button is different, timer is in a different location, exit button, etc.). Don't know how important these are to fix for v1, but I want to point them out nonetheless
  4. The counter of agenda items/slides at the bottom is not updated until you switch to the next slide. Should this also be updated automatically instead?
  5. Keyboard nav works great, but should there also be a way to exit via Esc?
  6. For longer text, the content is cut rather abruptly at the bottom. I don't know if we want to add a shadow effect or something? I quite like it as is, but maybe the designers think otherwise
  7. There's a bit of a flicker if you navigate to the next item if it was added by someone else once you're already presenting the current item. It first goes to the old next item before reloading to the new one. Don't know if this needs to be touched for now

Everything except the first two points can be dealt with later too, I think

@oliverguenther oliverguenther force-pushed the feat/meetings-presentation-mode branch 2 times, most recently from f41a923 to 947dd9e Compare October 31, 2025 09:57
@oliverguenther
Copy link
Member Author

Presenting an empty meeting needs to either not be possible or handled more gracefully. It returns a NoMethodError right now

Fixed

While editing an agenda item in presentation mode, if it is updated somewhere else, the current edit state is lost and the component is reloaded with the externally updated content. The reload shouldn't be happening at all when an item is being edited according to the AC

Fixed by having the auto-refresh only reload the show component part, this will prevent changing an active edit. You still get an error flash that the content has been changed (same as in regular meeting)

The UI doesn't exactly match the specs (presentation mode button is different, timer is in a different location, exit button, etc.). Don't know how important these are to fix for v1, but I want to point them out nonetheless

As discussed, waiting for Parimal to provide updates

The counter of agenda items/slides at the bottom is not updated until you switch to the next slide. Should this also be updated automatically instead?

As discussed, this was okay to be out of sync until you navigate

Keyboard nav works great, but should there also be a way to exit via Esc?

As discussed, this probably breaks with fullscreen mode anyway. I'll add that in a separate ticket to evalute

For longer text, the content is cut rather abruptly at the bottom. I don't know if we want to add a shadow effect or something? I quite like it as is, but maybe the designers think otherwise

As discussed, this is as expected

There's a bit of a flicker if you navigate to the next item if it was added by someone else once you're already presenting the current item. It first goes to the old next item before reloading to the new one. Don't know if this needs to be touched for now

This is coming from the turbo-prefetch on the links. We said we wanted to keep this for now, as it optimized for speed rather than correct content on quicker load.

@oliverguenther oliverguenther force-pushed the feat/meetings-presentation-mode branch from 947dd9e to 3c1cc51 Compare October 31, 2025 09:59
@mrmir
Copy link
Contributor

mrmir commented Oct 31, 2025

@oliverguenther it's possible to present a template. I don't think that makes sense to keep

@oliverguenther
Copy link
Member Author

@oliverguenther it's possible to present a template. I don't think that makes sense to keep

Indeed, fixed and specced 👍

@github-actions github-actions bot requested a deployment to gh-6899875-pr-20865 November 2, 2025 12:53 Pending
@github-actions github-actions bot temporarily deployed to gh-6899875-pr-20865 November 2, 2025 20:19 Inactive
@oliverguenther
Copy link
Member Author

@lindenthal I fixed the PullPreview deployment, it's now available here: https://pr-20865-meetings-present-ip-3-75-191-4.my.preview.run/projects/demo-project/meetings/2

@oliverguenther oliverguenther force-pushed the feat/meetings-presentation-mode branch from 4b55c61 to 4e34a4e Compare November 4, 2025 07:29
@github-actions github-actions bot temporarily deployed to gh-6899875-pr-20865 November 4, 2025 07:30 Inactive
@github-actions github-actions bot temporarily deployed to gh-6899875-pr-20865 November 4, 2025 19:59 Inactive
@oliverguenther oliverguenther force-pushed the feat/meetings-presentation-mode branch from 09b6af2 to a1c95dd Compare November 6, 2025 14:21
@github-actions github-actions bot temporarily deployed to gh-6899875-pr-20865 November 6, 2025 14:21 Inactive
@oliverguenther oliverguenther force-pushed the feat/meetings-presentation-mode branch from a1c95dd to 2c38962 Compare November 6, 2025 14:37
@github-actions github-actions bot temporarily deployed to gh-6899875-pr-20865 November 6, 2025 14:38 Inactive
Use separate start route so we can POST to it, indicating that it changes the meeting
Copy link
Contributor

@mrmir mrmir left a comment

Choose a reason for hiding this comment

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

It looks good overall, but I have a few questions:

  1. An empty section field is present when there are no sections in the meeting. Can this be hidden?
  2. For long agenda items that need to scroll, would it be possible to keep the header fixed like the footer and only scroll the content in between?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants