Skip to content

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Oct 17, 2024

This adds documentation for new feature integrated with Quarto

Copy link
Collaborator Author

@cderv cderv left a comment

Choose a reason for hiding this comment

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

@cwickham here is a first draft.

Several questions:

Copy link
Contributor

🚀 Deployed on https://deploy-preview-1387.quarto.org

@github-actions github-actions bot temporarily deployed to pull request October 17, 2024 13:50 Inactive
Copy link
Collaborator

@cwickham cwickham left a comment

Choose a reason for hiding this comment

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

Looking good! Regarding your questions:

  • This seems most related to navigation, so maybe it belongs closer to the top. Maybe after "Overview Mode"?
  • Yes, I think it needs a live example that someone can scroll. Maybe an iframe is the easiest?

Two other places we might need changes:

Copy link
Contributor

🚀 Deployed on https://deploy-preview-1387.quarto.org

@github-actions github-actions bot temporarily deployed to pull request October 18, 2024 09:30 Inactive
Copy link
Contributor

🚀 Deployed on https://deploy-preview-1387.quarto.org

@github-actions github-actions bot temporarily deployed to pull request October 18, 2024 09:44 Inactive
@cderv
Copy link
Collaborator Author

cderv commented Oct 18, 2024

I am now understanding we are hitting problem

and the freeze is not correctly updated to have this feature working in Quarto web;

@cderv
Copy link
Collaborator Author

cderv commented Oct 18, 2024

We need to wait for #1389 to be merged so that this feature is fully working

Copy link
Contributor

🚀 Deployed on https://deploy-preview-1387.quarto.org

@github-actions github-actions bot temporarily deployed to pull request October 18, 2024 13:06 Inactive
@cderv cderv force-pushed the revealjs/scroll-view branch from 6754f14 to b5c0a3b Compare October 18, 2024 13:44
Copy link
Contributor

🚀 Deployed on https://deploy-preview-1387.quarto.org

@github-actions github-actions bot temporarily deployed to pull request October 18, 2024 13:50 Inactive
@cderv
Copy link
Collaborator Author

cderv commented Oct 18, 2024

This section talks about swiping on mobile, which is now not the default interaction: quarto.org/docs/presentations/revealjs/advanced.html#touch-navigation

Regarding this, it seems tricky 🤔 It seems swipe is still activated for touch navigation, but now Horizontal and vertical swipe will do the same.

I can add a paragraph about scroll mode maybe 🤔 But it could be unclear to explain. Do you have a proposal ?

I tested https://prerelease.quarto.org/docs/presentations/revealjs/demo on mobile to check.

@cderv
Copy link
Collaborator Author

cderv commented Oct 18, 2024

/deploy-preview

Copy link
Contributor

🚀 Deployed on https://deploy-preview-1387.quarto.org

@cderv cderv marked this pull request as ready for review October 18, 2024 14:17
@cderv cderv requested a review from cwickham October 18, 2024 14:17
@cwickham
Copy link
Collaborator

I can add a paragraph about scroll mode maybe 🤔 But it could be unclear to explain. Do you have a proposal ?

I guess it's the second sentence in this bit I find a bit misleading:

You can swipe to navigate through a presentation on any touch-enabled device.
Horizontal swipes change between horizontal slides, vertical swipes change between vertical slides.

Because now there is no real distinction between vertical and horizontal swipes. So, maybe just deleting the second sentence is enough:

You can swipe to navigate through a presentation on any touch-enabled device.
If you wish to disable ...

Copy link
Collaborator

@cwickham cwickham left a comment

Choose a reason for hiding this comment

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

Looks great!

@cderv
Copy link
Collaborator Author

cderv commented Oct 18, 2024

So, maybe just deleting the second sentence is enough:

I see. Done.

Thanks a lot for the review!

@cderv cderv merged commit 0d85eaa into prerelease Oct 21, 2024
2 of 3 checks passed
@cderv cderv deleted the revealjs/scroll-view branch October 21, 2024 15:05
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