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

PdfViewer component #5676

Merged
merged 38 commits into from
Sep 6, 2024
Merged

PdfViewer component #5676

merged 38 commits into from
Sep 6, 2024

Conversation

stsrki
Copy link
Collaborator

@stsrki stsrki commented Aug 7, 2024

Closes #598

@stsrki stsrki requested a review from David-Moreira August 7, 2024 11:46
@stsrki stsrki changed the title PdfViewer PdfViewer component Aug 7, 2024
@stsrki
Copy link
Collaborator Author

stsrki commented Aug 8, 2024

It should be mostly done feature-wise. Please review.

stsrki added 3 commits August 8, 2024 12:20
# Conflicts:
#	Documentation/Blazorise.Docs/Pages/News/2024-10-15-release-notes-170.razor
@David-Moreira
Copy link
Contributor

Demo

  1. Can the container keep up with the updated pdf dimensions? So when I switch to 50% I still have a huge container, but it should be smaller wrapping around the size of the pdf?
    image
  2. Opportunity to show this working with our pager component? Either here or in docs or even both.
    image
  3. Both examples there's a vertical bar by default, which is understandable since the pdf doesn't fit. But are we able to show an example where it's able to fit automatically to the rendered size?

Docs:

  1. Have we tried a local source with a relative url? Any problem with local/relative urls?
  2. Since we are not using the anonymous delegate for anything can't we just bind directly? Clicked="delegate" Or you think it's best the anonymous delegate for end users?

image

  1. There's no binding example I think, does it work correctly both standalone changing things declaretively and with programmatic triggers? @bind-PageNumber @bind-Scale

@David-Moreira
Copy link
Contributor

State Class?
If we have a PdfViewerContainer that acts as a Parent for PdfViewerToolbar PdfViewer why do we need to define the state in both?
If the idea is to sync up, I guess the container should have this state, and it should be able to manage both internally.

If we allow a state for each one of the components, the user can just provide different states for both the toolbar and pdf viewer, is this ever what we want?

Also I opened the State class expect a bunch of state, and all I see is events? Are we sure we are conveying the expected meaning?

Copy link
Contributor

@David-Moreira David-Moreira left a comment

Choose a reason for hiding this comment

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

LGTM

@stsrki
Copy link
Collaborator Author

stsrki commented Sep 4, 2024

  1. Can the container keep up with the updated pdf dimensions? So when I switch to 50% I still have a huge container, but it should be smaller wrapping around the size of the pdf?

The container should keep the same size no matter the zoom level.

  1. Since we are not using the anonymous delegate for anything can't we just bind directly? Clicked="delegate" Or you think it's best the anonymous delegate for end users?

It might be a good thing to have. I'll see 🤔

3. Both examples there's a vertical bar by default, which is understandable since the pdf doesn't fit. But are we able to show an example where it's able to fit automatically to the rendered size?

For this we would have to have more work with the scaling math in the JS. For the first version of PdfViewer, I want to first publish it and then see what to focus on once the users start using it.

@stsrki
Copy link
Collaborator Author

stsrki commented Sep 4, 2024

  1. Have we tried a local source with a relative url? Any problem with local/relative urls?

As long as the user defines the absolute URL path this should work. Not sure if we need an example in the docs or leave it to the user to figure it.

@David-Moreira
Copy link
Contributor

Can the container keep up with the updated pdf dimensions? So when I switch to 50% I still have a huge container, but it should be smaller wrapping around the size of the pdf?

The container should keep the same size no matter the zoom level.

Is there a specific reason for this?

@David-Moreira
Copy link
Contributor

For this we would have to have more work with the scaling math in the JS. For the first version of PdfViewer, I want to first publish it and then see what to focus on once the users start using it.

I don't understand why you need to do calcs for this. Can't outside divs autosize according to the inner content dimensions? That is by using the appropriate css rules, of course.

@David-Moreira
Copy link
Contributor

David-Moreira commented Sep 4, 2024

  1. Have we tried a local source with a relative url? Any problem with local/relative urls?

As long as the user defines the absolute URL path this should work. Not sure if we need an example in the docs or leave it to the user to figure it.

Personally I'd expect it to work with relative urls, just like an image src or an anchor tag, but this isn't easily possible as I understand?

@stsrki
Copy link
Collaborator Author

stsrki commented Sep 5, 2024

For this we would have to have more work with the scaling math in the JS. For the first version of PdfViewer, I want to first publish it and then see what to focus on once the users start using it.

I don't understand why you need to do calcs for this. Can't outside divs autosize according to the inner content dimensions? That is by using the appropriate css rules, of course.

Because pdf.js works that way. We would need to calculate scale for the page viewport based on the canvas size, and page scrolling area. I checked into their own viewer code. And I tell you, it doesn't look friendly at all to figure it out.

@stsrki stsrki requested a review from David-Moreira September 5, 2024 09:25
@David-Moreira
Copy link
Contributor

For this we would have to have more work with the scaling math in the JS. For the first version of PdfViewer, I want to first publish it and then see what to focus on once the users start using it.

I don't understand why you need to do calcs for this. Can't outside divs autosize according to the inner content dimensions? That is by using the appropriate css rules, of course.

Because pdf.js works that way. We would need to calculate scale for the page viewport based on the canvas size, and page scrolling area. I checked into their own viewer code. And I tell you, it doesn't look friendly at all to figure it out.

I see. Thanks for explaining, sometimes things seem easier then what they are.

@stsrki stsrki merged commit f7b22ed into master Sep 6, 2024
2 checks passed
@stsrki stsrki deleted the dev-pdfviewer branch September 6, 2024 08:18
@github-actions github-actions bot locked and limited conversation to collaborators Sep 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PdfViewer component
2 participants