Skip to content

Text field#51

Open
flaiming wants to merge 7 commits intoFriendsOfFlarum:masterfrom
flaiming:text-field
Open

Text field#51
flaiming wants to merge 7 commits intoFriendsOfFlarum:masterfrom
flaiming:text-field

Conversation

@flaiming
Copy link
Copy Markdown

@flaiming flaiming commented Jul 5, 2020

I was lacking field for formatted text, so I added it :) It's not finished yet, but main functionality is there - you can add text field in Masquerade admin, you can fill it in user profile (with live preview of formatting) and it will display for other users formatted in this person profile.
I would just like to get help how to properly finish it:

  • As I understand, Flarum is storing formatted text to database in some intermediate XML format. But that seemed a bit too complicated so I'm storing original text with unrendered formatting directly into database. Is it OK? Why is storing Flarum formatted text as XML? Is there some security reason?

  • I have a little trouble with serializing Answer. I need to get field type in order to know if I need to render value into HTML or not. The way I currently do it seems a bit dirty.

  • Preview is rendering fine even with fof/upload extension, but final backend rendering does not know fof/upload format. How can I make backend HTML render to work with fof/upload format?

Please let me know if you can help me with any of that and what is your opinion on this new field.

@clarkwinkelmann
Copy link
Copy Markdown
Member

I'm not sure what to do here, and I have a bit of a conflict of interest.

I have implemented that exact same thing in my paid Formulaire extension and it took a lot of effort to get working properly. I just can't dedicate time to help get this working in Masquerade.

The reason for the XML storage of TextFormatter is performance. By storing an intermediate version in XML, TextFormatter does the heavy work a single time when saving a post, and rendering is then super-fast when doing the final transformation from XML to HTML. The performance gain is probably negligible for simple markdown/bbcode but is more important for features like mentions where some database queries must be done and the parse/render system allows some of the requests to only run once instead on every display of the post.

Flarum might not be a great example of the XML feature because we actually still perform a number of database queries during rendering. But that's the general idea and extensions can take advantage of that.

Maybe somebody else from the FriendsOfFlarum team will be interested in helping with this, but as we're all very busy I'm afraid this might take a while.

Feel free to use your own forked version on your forum in the meantime.

@flaiming
Copy link
Copy Markdown
Author

Thanks for explanation, I'll save it to database as XML.

As for conflict of interest, I think that Formulaire has way more functionality than Masquerade, so to me they are not really comparable. But I think I can find some way on my own.

I would just like to know if you have some requirements that needs to be met in order for you to accept this pull request?
Just now it displays textarea with live preview (in user profile to update value), but no toolbars for markdown etc. And rendering is not working with fof/upload. Other than that it works fine, that means you can set formatted text and it wil display it in user profile (as html).

@DavideIadeluca
Copy link
Copy Markdown
Member

Sorry for the huge delay @flaiming! Are you still looking to get this PR merged or not? If yes, It would probably be best if you start over with all the merge conflicts we have right now or rebase master into your branch.,

As @clarkwinkelmann already said, we are all quite busy and can't guarantee that this feature even would get properly reviewed and merged. With the Flarum 2.x Upgrade just in front of us, we are currently more or less in a feature freeze

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.

3 participants