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

working react example #52

Closed
wants to merge 2 commits into from
Closed

working react example #52

wants to merge 2 commits into from

Conversation

Vilhelm-Ian
Copy link

No description provided.

Copy link
Collaborator

@jamie-lemon jamie-lemon left a comment

Choose a reason for hiding this comment

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

Added some layout tips which should help.

The main thing though is - can we somehow reference what would exist in ../../dist for the libraries? We don't really want to duplicate this in public. If there is no way around it then update the README to inform the user about what they need to copy into "public".

height: 100%;
}

body {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change this to .App to target the React DOM class for better layout

}

header {
position: relative;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change to:

postion: sticky
top:0
z-index:1

For better layout

}

/* OUTLINE */

Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a new DOM target with:

#outline-panel {
  position: fixed;
  top: 25px;
  height: 100vh;
  overflow-y: scroll;
}

For better layout :)

page_count = await worker.countPages(current_doc)

// Use second page as default page size (the cover page is often differently sized)
var page_size = await worker.getPageSize(current_doc, page_count > 1 ? 2 : 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of index by 1. Change to 1 ? 1 : 0 ( note this is also a bug in the current simple viewer, 91228c4 )

<details>
<summary>View</summary>
<menu>
<li onClick={()=>Viewer.toggle_fullscreen()}>Fullscreen</li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Weird indentation from here ....

@Vilhelm-Ian
Copy link
Author

Added some layout tips which should help.

The main thing though is - can we somehow reference what would exist in ../../dist for the libraries? We don't really want to duplicate this in public. If there is no way around it then update the README to inform the user about what they need to copy into "public".

I couldn't find a way to get it to work without copying every file from dist

working react example
improved layout and changed readme
@jamie-lemon
Copy link
Collaborator

@Vilhelm-Ian Looking at what you have done here - I think the force pushes might have messed up the branch - if I look at the graphical diffs, i.e.: https://github.com/ArtifexSoftware/mupdf.js/pull/52/files I see lots of changes outside of where you should be making changes - all changes for your PR should just be to examples/mupdf-react. I think it might be easier to abandon this PR and do a fresh one!

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