-
Notifications
You must be signed in to change notification settings - Fork 7
Presentation mode for frames #916
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 15d068c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
60d24d1 to
a262d2a
Compare
eb11fee to
aa7d217
Compare
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
Signed-off-by: Mikhail Aheichyk <mikhail.aheichyk@nordeck.net>
34fe4b0 to
a2367b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks reasonable but I did add a few code comments for review.
Also, not related to code but to how things are implemented:
- it's very very slow when loading my MC'25 contents (30+ slide), opening and closing the sidebar takes a few seconds, as well as selecting a frame from the sidebar to zoom on it (see my comment on maybe not reusing
SvgCanvasfor previews) - the sidebar button is not show if there are no frames: I think we can show the sidebar and have helper text there that says "Please add a frame first to manage your presentation", maybe even keep the "Add frame" button (see code comment about this).
- more importantly, the presentation button should be hidden if there are no frames, or at least show a modal that also informs you that you need to add frames to present
- adding a frame near the left border is not doing a nice layout, it maybe needs to change line and start below on the right, if there is space available? to replicate, just keep adding frames in a blank whiteboard until they hit the left border.
| @@ -0,0 +1,6 @@ | |||
| --- | |||
| '@nordeck/matrix-neoboard-react-sdk': patch | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if patch or minor - we're not adding a feature to the app but we're enabling an existing feature to a new interaction model... ![]()
| export type SlideListItemProps = { | ||
| slideId: string; | ||
| slideOrFrameId: string; | ||
| slideIndex: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why change slideId and keep slideIndex ? also it's the SlideListItemProps...
| <Box m={1}> | ||
| <AddSlideButton /> | ||
| </Box> | ||
| {!isInfiniteCanvasMode() && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why disable the add slide button? Could we not add an empty frame to the canvas and centre on it, ready to add content?
| const position = useMemo(() => { | ||
| const pointOnSvg = { x, y }; | ||
| const elementOnContainer = transformPointSvgToContainer(pointOnSvg); | ||
| let elementOnContainer = transformPointSvgToContainer(pointOnSvg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get how these changes in the ElementBarWrapper are relevant within the context of this PR.
Are you fixing an unrelated bug? or what am I missing?
| containerDimensionsRef, | ||
| ]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've experienced severe slowness when having a board with many slides. Reusing SvgCanvas for frame previews and the full canvas is starting to feel a bit weird. A preview canvas won't be infinite, so reusing the same component doesn't seem optimal.
| containerDimensions.width > 0 && | ||
| containerDimensions.height > 0 | ||
| ) { | ||
| const { position, scale } = calculatePositionAndScaleForElement( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could abstract this block to yet another function with a name that is more expressive, such as zoomToElement or zoomOnElement or zoomFitToElement, which expresses more clearly what's going on and could be reused on other use cases, such as:
- URL deep links to elements
- "center on element" feature
- "follow cursor" feature
no strong feelings, just an opinion.
| "pdfError": "Bei der Erstellung des PDFs ist ein Fehler aufgetreten.", | ||
| "title": "Inhalt exportieren" | ||
| }, | ||
| "hideFrameBarTitle": "Close frame overview", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing translations in this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "hideFrameBarTitle": "Close frame overview", | |
| "hideFrameBarTitle": "Rahmenübersicht schließen", |
| } | ||
|
|
||
| startPresentation(): void { | ||
| startPresentation(activeFrameElementId?: string): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no new tests for starting a presentation with a frame - should we not include that as well?
HarHarLinks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically some of these could be refactored to deduplicate :P
i hope applying the proposed changes via the review might confuse doesn't confuse the hell out of the i18n framework.
| "pdfError": "Bei der Erstellung des PDFs ist ein Fehler aufgetreten.", | ||
| "title": "Inhalt exportieren" | ||
| }, | ||
| "hideFrameBarTitle": "Close frame overview", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "hideFrameBarTitle": "Close frame overview", | |
| "hideFrameBarTitle": "Rahmenübersicht schließen", |
| "import": "Importieren…", | ||
| "title": "Einstellungen" | ||
| }, | ||
| "showFrameBarTitle": "Open frame overview", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "showFrameBarTitle": "Open frame overview", | |
| "showFrameBarTitle": "Rahmenübersicht öffnen", |
| "nextFrame": "Next frame", | ||
| "nextSlide": "Nächste Folie", | ||
| "previousFrame": "Previous frame", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "nextFrame": "Next frame", | |
| "nextSlide": "Nächste Folie", | |
| "previousFrame": "Previous frame", | |
| "nextFrame": "Nächster Rahmen", | |
| "nextSlide": "Nächste Folie", | |
| "previousFrame": "Vorheriger Rahmen", |
| "dragAndDrop": { | ||
| "dragInstructions": "Drücke die M-Taste um diese Folie zu verschieben. Während des Verschiebens, nutze die Pfeiltasten um den Eintrag zu verschieben und Escape um es abzubrechen. Stelle sicher das dein Screen Reader im Fokus oder Formular Modus ist.", | ||
| "dragStart": "Du hast eine Folie aufgehoben. Sie befindet sich an Position {{startPosition}} von {{totalCount}} in der Liste. Nutze die Pfeiltasten zum verschieben, die M-Taste zum Ablegen und Escape zum Abbrechen.", | ||
| "dragStart_frame": "You have lifted a frame. It is in position {{startPosition}} of {{totalCount}} in the list. Use the arrow keys to move, the M key to drop, and escape to cancel.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "dragStart_frame": "You have lifted a frame. It is in position {{startPosition}} of {{totalCount}} in the list. Use the arrow keys to move, the M key to drop, and escape to cancel.", | |
| "dragStart_frame": "Du hast einen Rahmen aufgehoben. Er befindet sich an Position {{startPosition}} von {{totalCount}} in der Liste. Nutze die Pfeiltasten zum verschieben, die M-Taste zum Ablegen und Escape zum Abbrechen.", |
| "dragStart": "Du hast eine Folie aufgehoben. Sie befindet sich an Position {{startPosition}} von {{totalCount}} in der Liste. Nutze die Pfeiltasten zum verschieben, die M-Taste zum Ablegen und Escape zum Abbrechen.", | ||
| "dragStart_frame": "You have lifted a frame. It is in position {{startPosition}} of {{totalCount}} in the list. Use the arrow keys to move, the M key to drop, and escape to cancel.", | ||
| "dropped": "Du hast die Folie abgelegt. Sie wurde von Position {{startPosition}} nach {{destinationPosition}} verschoben.", | ||
| "dropped_frame": "You have dropped the frame. It has moved from position {{startPosition}} to {{destinationPosition}}.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "dropped_frame": "You have dropped the frame. It has moved from position {{startPosition}} to {{destinationPosition}}.", | |
| "dropped_frame": "Du hast den Rahmen abgelegt. Er wurde von Position {{startPosition}} nach {{destinationPosition}} verschoben.", |
| "dropped": "Du hast die Folie abgelegt. Sie wurde von Position {{startPosition}} nach {{destinationPosition}} verschoben.", | ||
| "dropped_frame": "You have dropped the frame. It has moved from position {{startPosition}} to {{destinationPosition}}.", | ||
| "droppedOnNoDropTarget": "Du hast die Folie abgelegt während du nicht über einer gültigen Ablagestelle gewesen bist. Die Folie ist zu ihrer ursprünglichen Position {{startPosition}} zurückgekehrt.", | ||
| "droppedOnNoDropTarget_frame": "The frame has been dropped while not over a location. The frame has returned to its starting position of {{startPosition}}.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "droppedOnNoDropTarget_frame": "The frame has been dropped while not over a location. The frame has returned to its starting position of {{startPosition}}.", | |
| "droppedOnNoDropTarget_frame": "Du hast den Rahmen abgelegt während du nicht über einer gültigen Ablagestelle gewesen bist. Der Rahmen ist zu seiner ursprünglichen Position {{startPosition}} zurückgekehrt.", |
| "droppedOnNoDropTarget": "Du hast die Folie abgelegt während du nicht über einer gültigen Ablagestelle gewesen bist. Die Folie ist zu ihrer ursprünglichen Position {{startPosition}} zurückgekehrt.", | ||
| "droppedOnNoDropTarget_frame": "The frame has been dropped while not over a location. The frame has returned to its starting position of {{startPosition}}.", | ||
| "movedPosition": "Du hast die Folie zu Position {{position}} von {{totalCount}} verschoben.", | ||
| "movedPosition_frame": "You have moved the frame to position {{position}} of {{totalCount}}.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "movedPosition_frame": "You have moved the frame to position {{position}} of {{totalCount}}.", | |
| "movedPosition_frame": "Du hast den Rahmen zu Position {{position}} von {{totalCount}} verschoben.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing translation on line 303 for "droppedOnNoDropTarget_frame"?
| "movedPosition": "Du hast die Folie zu Position {{position}} von {{totalCount}} verschoben.", | ||
| "movedPosition_frame": "You have moved the frame to position {{position}} of {{totalCount}}.", | ||
| "movementCanceled": "Verschieben abgebrochen. Die Folie ist zu ihrer ursprünglichen Position {{startPosition}} zurückgekehrt.", | ||
| "movementCanceled_frame": "Movement cancelled. The frame has returned to its starting position of {{startPosition}}.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "movementCanceled_frame": "Movement cancelled. The frame has returned to its starting position of {{startPosition}}.", | |
| "movementCanceled_frame": "Verschieben abgebrochen. Der Rahmen ist zu seiner ursprünglichen Position {{startPosition}} zurückgekehrt.", |
| "frameListTitle": "Frames", | ||
| "frameTitle": "Frame", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "frameListTitle": "Frames", | |
| "frameTitle": "Frame", | |
| "frameListTitle": "Rahmen", | |
| "frameTitle": "Rahmen", |
| "slideTitleLocked": "(gesperrt)", | ||
| "title": "Folienübersicht" | ||
| "title": "Folienübersicht", | ||
| "title_frame": "Frame Overview" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "title_frame": "Frame Overview" | |
| "title_frame": "Rahmenübersicht" |
Enables frames overview and provides a presentation mode for the frames:
✔️ Checklist
Signed-off-byline in the message (more info).