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

Text content not always editable #576

Closed
ryandeba opened this issue Nov 27, 2017 · 11 comments
Closed

Text content not always editable #576

ryandeba opened this issue Nov 27, 2017 · 11 comments
Labels

Comments

@ryandeba
Copy link
Contributor

ryandeba commented Nov 27, 2017

Hi @artf,

I've noticed that if I add a component that contains a combination of text and an <a> tag, the text is not editable like it normally is. If I replace the <a> tag with a <span>, then I can edit the text just fine. Here's something to illustrate what I mean (note the last 2 lines of code to toggle between a broken example and a working example): https://jsfiddle.net/wycrpkay/3/. This may be happening with tags other than <a>, but that's the only one I've noticed to be problematic so far.

It looks to me like the component is getting parsed as the default component type instead of a text component, but I cannot figure out why the presence of the <a> tag (and not other tags) would cause this to happen. I saw that ComponentText.js does not have an isComponent function, so I'm not quite understanding how that distinction is made. I'm going to keep looking into this, but would appreciate any thoughts you may have on why this would be happening.

Thanks!

EDIT: So I found ParserHtml.parseNode()...adding comp.type != 'link' to the if statement starting on line 183 seems to do what I want:
if(comp.type != 'text' && comp.type != 'textnode' && comp.type != 'link' && c.textTags.indexOf(comp.tagName) < 0 ){
I don't feel great about that change though. I feel like I want those checks to just see if all of the children are editable (comp.get("editable")) instead of all the text/textnode/link checks, but that doesn't seem possible here since we just have simple component objects (looks like it's just whatever is returned from the isComponent functions). Not sure what to do...

@duskhacker
Copy link

I'm running into this too.

@duskhacker
Copy link

As a workaround, i've been adding a class div.inline{ display: inline} to isolate the <a> tags until this gets fixed.

@ryandeba
Copy link
Contributor Author

ryandeba commented Dec 1, 2017

Requirements for the project that I'm working on have changed which will likely make this issue irrelevant for me (all components are editable: false, except for a new custom component that will likely use CKEditor for the RTE).

I think part of the problem (or possibly just confusion on my part) is due to the differences in how the ComponentTextView object parses child components after editing content via the RTE vs how the ParserHTML object parses components from HTML being set into the editor. The former leaves the parent component editable after links (or other HTML elements) are added through rich text editing, while the latter can leave text in an uneditable state if it contains links (or other non-text components). Now that I've got a better understanding of what's happening, it makes more sense to me. I think it's problematic if you need to set raw HTML content into the editor if that content has text that is a sibling to one or more non-text elements, but I don't know a good way to fix it.

@artf
Copy link
Member

artf commented Dec 1, 2017

Hi @ryandeba the problem I was facing is how to understand where to declare a component a Text Component, for instance:

1. Element with a single textnode inside

<div> Simple </div>

No problem, I mark the div as a text and this will be editable by the user.

2. Element with nested elements

<div>
     <div>El1</div>
     <div>El2</div>
</div>

Ok El1 and El2 are just like above (ediatble text components) but our main div is a container so it will be a simple component (not a text one)

3. Element with nested mixed elements

<div>
     Some textnode
     <b>here</b>
     and
     <i>here</i>
</div>

Ok here we find textnodes (['Some textnode', 'and']) and other elements (['<b>here</b>', '<i>here</i>']). Well, we can easily say that elements like <b>, <i>, <u>, etc. are editable too therefore I've added this option in parser config and use it with c.textTags.indexOf(comp.tagName) < 0
note: I'll add <a> in the next release so this issue will be fixed

4. Element with nested mixed elements 2

<div>
     Some textnode
     <div>...</div>
</div>

What about this???
There is another div inside, which can contain other complex components so I can't mark the main <div> as text, but there is a textnode which becomes uneditable. The only solution I see would be to wrap that textnode (eg. span) but not sure if it's a good thing changing the structure (eg. there is a CSS like span { color: red })

@duskhacker
Copy link

@artf IMHO, instance 4 is an error. I know that it's valid HTML, but for the purpose of "GrapesJS-flavored-HTML" Some textnode should be wrapped in a div, or other editable node type. That makes sense (to me anyway) from the perspective of looking at/writing the raw HTML. Again IMHO, the program shouldn't change the structure of the HTML, possibly it could just log an error to the console, as I think that people that would be looking at the raw HTML would be those that are implementing GrapesJS in their app and adapting legacy documents to it, so would be aware of the console and messages written to it. Documentation could point to that too to drive the point home.

There are two situations I can think of, 1) A user is adapting legacy HTML to the new format. 2) The HTML is generated by the program in the first place, (in the case of people using it to actually create a page, which is the reason this thing exists).

In case 1, clear rules can/should be published for morons like me that need to adapt their legacy stuff. One of those rules could be "all text needs to be wrapped in an editable component". In case 2, the program should just generate the "correct" HTML in the first place, that is, each text is wrapped in an editable node.

@artf
Copy link
Member

artf commented Dec 1, 2017

Thanks Daniel, I agree with you and will take care of your suggestions.
I just would like to point out, for the 4th case, that you can also indicate explicitly to the editor where is a text component

<div data-gjs-type="text">
     Some textnode
     <div>Text to edit</div>
     <ul>...</ul>
</div>

@duskhacker
Copy link

@artf

you can also indicate explicitly to the editor where is a text component

Thank you! That actually helps me a lot until the fixes are released.

@artf
Copy link
Member

artf commented Dec 1, 2017

Should be fixed https://github.com/artf/grapesjs/releases/tag/v0.12.50

@artf artf closed this as completed Dec 1, 2017
@ryandeba
Copy link
Contributor Author

ryandeba commented Dec 2, 2017

Thank you @artf!

@Siddharth-ss42279
Copy link

Hey @artf @ryandeba , actually i am using ckeditor.

After editing content inside text block the 4th case is occuring and Some textnode becomes un-editable.

In the above case how do i make content editable. i want to open ckeditor or RTE (if possible) on that text also.
can you tell how can i make the content editable explicitly?. i need help with some sample code?

@lock
Copy link

lock bot commented Oct 20, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the outdated label Oct 20, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants