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

[sveltekit] Text view #532

Merged
merged 17 commits into from
Apr 30, 2024
Merged

[sveltekit] Text view #532

merged 17 commits into from
Apr 30, 2024

Conversation

eyeseast
Copy link
Collaborator

@eyeseast eyeseast commented Apr 26, 2024

@eyeseast eyeseast requested a review from allanlasser April 26, 2024 16:04
Copy link

netlify bot commented Apr 26, 2024

Deploy Preview for documentcloud-frontend ready!

Name Link
🔨 Latest commit ba89e3f
🔍 Latest deploy log https://app.netlify.com/sites/documentcloud-frontend/deploys/663128c6173aa80008a31628
😎 Deploy Preview https://deploy-preview-532.muckcloud.com
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@allanlasser
Copy link
Member

allanlasser commented Apr 26, 2024

Awesome start. Initial thoughts:

  • Since I'm working in Figma now, I'm gonna mock up some styling for this. Most important styling consideration is typography: I think this is a good time to add Source Code Pro as a @font-face.
  • For finding the text view, I think it's a good time to add a footer Toolbar to the ContentLayout that contains a "view" menu. We can reuse the src/common/Dropdown2 component for menu logic (that's what I used in the global nav). It has a prop control the placement of the menu in relation to the menu button.
  • If it's easy to track the current page in the text view, we should write that to some viewer state and handle pagination. This already exists in the current viewer.
  • If the text view is a state of the document, then it may be better as a search param than a path. If the view is a resource of the document, then having /text in the path is better. Both approaches are equivalent, so having a stated reason will help with Finalize URL structure #522

Do we need to render notes, redactions, or other edits onto the text?

Copy link

github-actions bot commented Apr 26, 2024

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 37.9% 2892 / 7630
🔵 Statements 37.9% 2892 / 7630
🔵 Functions 26.87% 43 / 160
🔵 Branches 47.53% 106 / 223
File Coverage
File Stmts % Branch % Funcs % Lines Uncovered Lines
Changed Files
src/lib/api/documents.ts 82.29% 79.31% 83.33% 82.29% 27-55, 61-90, 120-121, 125-126, 132-133, 166-167, 228
src/lib/api/types.d.ts 0% 0% 0% 0% 1-226
src/lib/components/documents/TextPage.svelte 0% 0% 0% 0% 1-55
src/routes/app/+page.svelte 0% 0% 0% 0% 1-119
src/routes/app/add-ons/+page.svelte 0% 0% 0% 0% 1-131
src/routes/app/projects/+page.svelte 0% 0% 0% 0% 1-64
src/routes/app/projects/[id]-[slug]/+page.svelte 0% 0% 0% 0% 1-112
src/routes/documents/[id]-[slug]/+layout.ts 0% 0% 0% 0% 1-43
src/routes/documents/[id]-[slug]/+page.svelte 0% 0% 0% 0% 1-177
src/routes/documents/[id]-[slug]/+page.ts 0% 0% 0% 0% 1-27
Generated in workflow #89

@eyeseast
Copy link
Collaborator Author

If the text view is a state of the document, then it may be better as a search param than a path. If the view is a resource of the document, then having /text in the path is better. Both approaches are equivalent, so having a stated reason will help with #522

Text has to be fetched on its own, after we fetch the document data (because we need document.asset_url), so that makes me think it's better giving it a dedicated URL so we can fetch it server-side.

We have four document modes:

  • document
  • text
  • thumbnails
  • notes (if there are notes)

Those could all be at the same URL with query params to switch. Text is the only resource that needs to be fetched on its own.

@eyeseast eyeseast changed the title First pass at text view [sveltekit] Text view Apr 30, 2024
public/fonts.css Show resolved Hide resolved
src/lib/api/documents.ts Outdated Show resolved Hide resolved
src/lib/components/documents/TextPage.svelte Show resolved Hide resolved
src/lib/components/documents/TextPage.svelte Show resolved Hide resolved
src/lib/components/documents/TextPage.svelte Outdated Show resolved Hide resolved
src/routes/documents/[id]-[slug]/+page.svelte Show resolved Hide resolved
src/routes/documents/[id]-[slug]/+page.ts Outdated Show resolved Hide resolved
src/routes/documents/[id]-[slug]/+page.svelte Show resolved Hide resolved
@eyeseast eyeseast requested a review from allanlasser April 30, 2024 15:56
Copy link
Member

@allanlasser allanlasser left a comment

Choose a reason for hiding this comment

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

These styling changes should fix the width/layout of the pages inside the viewer. Everything else looks great!

src/routes/documents/[id]-[slug]/+page.svelte Outdated Show resolved Hide resolved
src/routes/documents/[id]-[slug]/+page.svelte Show resolved Hide resolved
src/lib/components/documents/TextPage.svelte Outdated Show resolved Hide resolved
Copy link
Member

@allanlasser allanlasser left a comment

Choose a reason for hiding this comment

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

All code looks great. I caught one bug while testing—I will leave it up to you whether you want to fix it here, or create an issue targeting it.

When trying to load a specific page by target (i.e. refreshing a URL ending in #p10), the element is not brought into view. I believe the issue is that the element being targeted is not present when the page loads—it's loaded in after. There's a small flash where the mode is "Document" and the view is empty, before switching to "Text" and displaying the text pages. Either the page should be scrolled into view after streaming finishes (example) or we should change how we're server-rendering the page.

@eyeseast
Copy link
Collaborator Author

Let's merge this and then deal with the page hash when we do pagination, which I think will end up addressing that.

I'm not sure what's happening with mode and the initial state.

@eyeseast eyeseast merged commit c600c52 into sveltekit Apr 30, 2024
10 checks passed
@allanlasser allanlasser mentioned this pull request May 1, 2024
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