Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

Fixes Page Scroll Issue #87

Merged
merged 6 commits into from
Sep 1, 2020
Merged

Fixes Page Scroll Issue #87

merged 6 commits into from
Sep 1, 2020

Conversation

CaptainDredge
Copy link

@CaptainDredge CaptainDredge commented Aug 14, 2020

Fixes #81 Fixes #82

In this PR I've tried to solve issue #81 and #82 by making the whole layout of pages discrete rather than continuous ones. It has several advantages in terms of performance as well as user interaction.

@netlify
Copy link

netlify bot commented Aug 14, 2020

Deploy preview for text-to-handwriting processing.

Building with commit 9ce4928

https://app.netlify.com/sites/text-to-handwriting/deploys/5f4526fc88cd110007b7bc70

@saurabhdaware
Copy link
Owner

Thank you for the PR @CaptainDredge. I haven't checked PR properly but we can't paste content inside page. It is throwing some error in console. Can you check?

Also, is there any reason why it is using jQuery and not JavaScript. It is better to avoid additional dependencies as it will make code complicated to maintain. Can it be done without jQuery?

Use of Jquery adds an additional prototype in getting clipboard data from event
 Please enter the commit message for your changes. Lines starting
@CaptainDredge
Copy link
Author

@saurabhdaware I've corrected the paste event handler. Also, Jquery is necessary for event delegation in dynamically added pages. There's neither an easy nor an elegant solution of this in pure javascript.

@CaptainDredge
Copy link
Author

@saurabhdaware Have you looked into the PR now?

@saurabhdaware
Copy link
Owner

Hi there, I am on a break right now so I will try to check it by the next week. I hope it is fine with you. Thanks!

@saurabhdaware
Copy link
Owner

Hi, I just checked in the preview. It is not working when we have text larger than the area. Normally it is suppose to generate multiple images when a user pastes large text in the textarea.

@CaptainDredge
Copy link
Author

No according to updated functionality user can paste as much text as he wants but page will contain only the text it can accommodate on one page. Then you've to add another page to paste the remaining text. And that's how even actual pages in notebook works. Adding the functionality of accommodating all the text and moving it to the next pages according to current status of page capacity itself is a pretty complicated task and that'll essentially make it like MS word.

@saurabhdaware
Copy link
Owner

No, I meant pasting large text at once. It was one of the reasons to have a scrollbar so that they can paste in one box. There were lots of people initially who wanted a way to generate multiple images in a single paste so the usecase is too common to ignore

Added the functionality of automatically
creating pages when the content becomes
more than the current page capacity
@CaptainDredge
Copy link
Author

CaptainDredge commented Aug 23, 2020

@saurabhdaware Added that functionality although I had to do a lot of thinking behind implementing it 😅
You can now paste large text and it'll generate new pages accordingly. Also, when typing when the current page gets filled it'll generate the next page automatically. If the pasted content is too large it does have some performance implications associated with it.

@saurabhdaware
Copy link
Owner

This looks amazing!

Few things are not working as expected:
https://res.cloudinary.com/saurabhdaware/video/upload/v1598336022/photos/2020-08-25_11-40-06.mp4

  • While typing if I press the spacebar, it considers it as a line break and takes the cursor to next line
  • When I try to CTRL + C to copy the selected section, it removes the selection
  • Backspace is not working after selecting text content.

Let me know if it is possible to fix this otherwise maybe I can take this code to separate branch and see if I can do something.

Thanks and not sure if adding page below was intended but it actually looks great! maybe we can have a scrollbar like we had before but have multiple pages in the scroll instead a single section. That would look great

@CaptainDredge
Copy link
Author

I think its happening because of the event handlers attached to key down. But the keys pressed here like ^C and backspace are not meant to trigger those callbacks attached to those handlers. The first issue where spacebar is adding line breaks is not there in Google chrome which I use but its there in Firefox. I think there's some cross-browser compatibility issue in some function used in code which seems tough to figure out. You can definitely take the code to a separate branch to figure things out especially the first issue.

@CaptainDredge
Copy link
Author

CaptainDredge commented Aug 28, 2020

@saurabhdaware have checked on the first issue? I had already resolved the other two

@saurabhdaware
Copy link
Owner

not yet. I'll merge the PR in separate branch and will check what I can do. Thank youu!

@saurabhdaware saurabhdaware changed the title Fixes: #81 and #82 Fixes Page Scroll Issue Sep 1, 2020
Copy link
Owner

@saurabhdaware saurabhdaware left a comment

Choose a reason for hiding this comment

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

Thank you so much! I'll see what we can do. Thank you.

@saurabhdaware saurabhdaware changed the base branch from master to page-scroll-issue September 1, 2020 14:45
@saurabhdaware saurabhdaware changed the base branch from page-scroll-issue to master September 1, 2020 14:46
@saurabhdaware saurabhdaware changed the base branch from master to page-scroll-issue September 1, 2020 14:47
@saurabhdaware saurabhdaware merged commit 48dd805 into saurabhdaware:page-scroll-issue Sep 1, 2020
@CaptainDredge
Copy link
Author

So will I be added to the contributor list or not?

@CaptainDredge CaptainDredge deleted the bugfix branch September 1, 2020 15:13
@saurabhdaware
Copy link
Owner

Yes you will be added to contributor list. The list on the website comes from GitHub API so the name will be reflect there after I merge this new branch into master. I will have to make some changes before that.

@prafulla-codes
Copy link
Contributor

@saurabhdaware can you let me know what are the issues here?

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.

Left Margin doesn't scale well with body content Text from Margins is printed on every page.
3 participants