Skip to content

Comments

Added Custom Insert Module#568

Merged
jywarren merged 9 commits intopubliclab:mainfrom
keshav234156:Customtext
Aug 11, 2020
Merged

Added Custom Insert Module#568
jywarren merged 9 commits intopubliclab:mainfrom
keshav234156:Customtext

Conversation

@keshav234156
Copy link
Member

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with grunt jasmine
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • PR body includes fixes #0000-style reference to original issue #
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

@gitpod-io
Copy link

gitpod-io bot commented Jul 21, 2020

@keshav234156 keshav234156 mentioned this pull request Jul 21, 2020
5 tasks
@keshav234156
Copy link
Member Author

Screenshot from 2020-07-21 16-32-58
Locally the test output

@keshav234156
Copy link
Member Author

keshav234156 commented Jul 21, 2020

@jywarren @sagarpreet-chadha @NitinBhasneria @Shreyaa-s @Shulammite-Aso Please review. We can leave the passing of jest-puppeteer test as of now. The test are correct and are passing locally but it look like travis run with --runInBand to fasten te process which fails the test.locally it can be simulated by node ./node_modules/.bin/jest --runInBand .
Also, I have made this new PR (previous PR #538 ) because my previous forked repo was having too much problem with test and all branches were messed up (I don't why confused ?).

@cypherean
Copy link
Contributor

I just need to make one testing commit. It's working in mine. I hope that's okay @keshav234156 ?
Screenshot from 2020-07-21 16-46-35

@cypherean
Copy link
Contributor

Didn't work.

@jywarren
Copy link
Member

I just got to testing this in GitPod! Below is my screenshot. But, i tried using the insert button right where my text says Testing here -- but my insert got inserted at the top. This would be a really great opportunity to add a Jest test, demonstrating that if we move the insertion point to the middle of a body of text, the insert should be inserted there. We could assert that the final markdown text confirms that!

image

I was also able to confirm that if you just start typing in a blank editor, and try adding the insert at the end, it still gets put at the top. So I think that may be an easier test to write, actually?

editor3

@keshav234156
Copy link
Member Author

Actually, the result doesn't depend on where your pointer was at the start. It depends where you point after you given the inputs
Untitled_ Jul 22, 2020 9_25 AM (1)

@keshav234156
Copy link
Member Author

I tried implementing to make it insert where the pointer was at the start which would much more intuitive. But wasn't able to do so

Copy link
Contributor

@sagarpreet-chadha sagarpreet-chadha left a comment

Choose a reason for hiding this comment

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

Some minor changes,
Also you may want to checkout this on how to add text to caret location.
https://stackoverflow.com/questions/946534/insert-text-into-textarea-with-jquery/946556#946556

Let me know if this link works for you, meanwhile i will read wysiwyg api if this is possible easily.

@cypherean
Copy link
Contributor

@keshav234156 @jywarren @sagarpreet-chadha please have a look. I made a testing commit here (sorry Keshav) and the tests passed!

@keshav234156
Copy link
Member Author

@Shreyaa-s Good work for digging up and fixing the test properly!!

@keshav234156
Copy link
Member Author

@sagarpreet-chadha I tried a lot through the process given in the link. It worked well for markdown mode but not for rich text mode it didn't work. The main difference between them is in markdown mode the text is written in <textarea> whereas in rich mode text is written in content editable <div>. I have gone through various methods for content editable <div> but none of them worked. Have you done something like getting caret position in content editable

@sagarpreet-chadha
Copy link
Contributor

sagarpreet-chadha commented Jul 25, 2020

@Shreyaa-sharmaa good catch 🎉
@keshav234156 can you commit code for markdown mode which is working? I will try digging for
<div>, but ideally there is no difference between textarea and div, using JS we can manipulate innerHTML in both.

@keshav234156
Copy link
Member Author

@sagarpreet-chadha ok doing it!!

@keshav234156
Copy link
Member Author

@sagarpreet-chadha Please have a look. the main concern for <div> is getting selectionstart and selectionend, which is undefined.

@sagarpreet-chadha
Copy link
Contributor

Hi Keshav, glad it is working for markdown 🎉
Okay, so if i am writing something it has to be either input or textarea, we cannot write inside a div.
So in rich mode, you need to find the class of that rich module text area, either in code or by inspecting the web page and then finding the classname by hovering over carrat.
I hope this works otherwise we will need to think something else! No worries, great work so far 🎉 💯

@keshav234156
Copy link
Member Author

@sagarpreet-chadha No, it's not <input> and <textarea> . It's a contenteditable <div>. There are various methods for contenteditable div, but they only work when there is some selection

@sagarpreet-chadha
Copy link
Contributor

Hi Nitin, okay that makes sense! I found one solution here: https://stackoverflow.com/questions/2920150/insert-text-at-cursor-in-a-content-editable-div
Can you try using this function, it is same as what you did for markdown but instead if getting offset in markdiwn, here we need to get number of nodes in the div after which we have to add.Thanks!

@keshav234156
Copy link
Member Author

@sagarpreet-chadha the main problem with this is that sel = window.getSelection();. when we type input the caret is changed to where we are typing input. if instead, we change it to sel = this.getSelection(); then it comes to be undefined

@jywarren
Copy link
Member

Hi, i'm thinking - surely there is a standard woofmark way to insert at the cursor location regardless of mode? Yes, how about using the chunks object, which is described here -- can we use that? https://github.com/jywarren/woofmark/#chunks

@keshav234156
Copy link
Member Author

@jywarren No that doesn't work because when we are typing input then caret position is changed. that how I have implement and its works like this as shown above #568 (comment)

@jywarren
Copy link
Member

Oh, i see! Strange! Um, so, the start position is then lost?

Actually i believe the chunks state is saved for all previous states, because that's how woofmark's "undo" function works. Could it be accessed that way?

@sagarpreet-chadha
Copy link
Contributor

Okay @keshav234156 , I got the problem so when we are clicking go button, the carat position is lost, right?
can we solve this in following way:

  1. Can we have a listener that is triggered while typing anything. (Inside that event listener, use runcommand function as you are already doing in .go button click)
  2. We will have a global variable that will store the latest chunk object that we will get from above listener execution.
  3. Now when .go button is clicked we will use the stored chunk object (and not the one recieved currently)
  4. Using this stored chunk object, we can append at carat position.

@jywarren what do you think?

@jywarren
Copy link
Member

let's try it! Great thinking, @sagarpreet-chadha !

@keshav234156
Copy link
Member Author

@sagarpreet-chadha Please review!!
By default if there is was no previous caret then it would insert at the start position else at where there was a caret.

Copy link
Contributor

@sagarpreet-chadha sagarpreet-chadha left a comment

Choose a reason for hiding this comment

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

Great work so far 🎉
Can you send a working gif of insert at caret please. Thanks 😄
It may not work in firefox as written in mdn documentation.

@jywarren
Copy link
Member

jywarren commented Aug 4, 2020

Great! I'll give this a try in gitpod!!! https://gitpod.io/#https://github.com/publiclab/PublicLab.Editor/pull/568

@jywarren
Copy link
Member

jywarren commented Aug 4, 2020

This works really well. Thanks a ton! It does seem to strangely select some random text above but the grid is inserted in the right place:

image

Let's move to merge this and we can continue refining the interface!

Copy link
Member

@jywarren jywarren left a comment

Choose a reason for hiding this comment

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

Just one small change on modularizing the code into 2 files for readability. Then we can merge!

@keshav234156
Copy link
Member Author

This works really well. Thanks a ton! It does seem to strangely select some random text above but the grid is inserted in the right place:

image

Let's move to merge this and we can continue refining the interface!

This is because after plane text is inserted in rich mode. To apply the styling it going to markdown mode and then back to rich mode. It' s a separate issue. We can fix this in follow-ups!!

@keshav234156
Copy link
Member Author

I will fix #546 on similar lines once this is merged.

@jywarren
Copy link
Member

jywarren commented Aug 6, 2020

Hmm. Looks like one Travis run is stuck. I'll try restarting it...

@jywarren
Copy link
Member

jywarren commented Aug 6, 2020

Done!!!! Great work, @keshav234156 !!!! 🎉

This can be released with v2.2.0 after we release patch v2.1.1 in publiclab/plots2#8135 (comment) as mentioned. If @NitinBhasneria or you can file the fix and bump the package.json version for v2.1.1 then let's merge this just after that.

@jywarren
Copy link
Member

Updating for inclusion in v2.1.2!! Keshav, can you add a one-line description there to be included in the release notes? Thank you and congrats!

@jywarren jywarren merged commit c390a22 into publiclab:main Aug 11, 2020
@sagarpreet-chadha
Copy link
Contributor

Awesomeee great work here @keshav234156 , looking forward to see this feature live very soon 🎉

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.

4 participants