Render Stream Descriptions from the Backend - Frontend and UI Update #11480
Conversation
Use the results of commit #73d26c8 to remove the method `render_stream_description` in static/js/stream_data.js and instead use the rendered_description attribute now being sent by the backend. This will be a valuable optimization and a step towards removing the need for the marked.js markdown parser and speeding up the client end.
On clicking the edit button for a stream description, the stream's unrendered description should be made editable as text instead of the stream's rendered description (which would be displayed as HTML instead of text).
|
@timabbott could you please review this? |
| exports.render_stream_description(sub); | ||
| if (sub.rendered_description !== undefined) { | ||
| sub.rendered_description = sub.rendered_description.replace('<p>', '').replace('</p>', ''); | ||
| } |
There was a problem hiding this comment.
Why do we need to remove the p type tags here? I'm a little skeptical that doing so is correct (though I see we had it in the previous implementation).
There was a problem hiding this comment.
Without stripping the <p>, </p> tags, the edit symbol/button would get moved to the next line.
Like so:
Which is kinda ugly.
There was a problem hiding this comment.
I see. I think that's fine, what I don't like about this model is that we strip the <p> tags every time we call this function, regardless of whether the rendered_description has actually changed. It's definitely a code smell, even if it happens to be correct.
I think we could potentially address this in other ways, though -- e.g. either making the UI layer insert the edit widget inside the <p> tag, or tweaking the CSS. @synicalsyntax do you see a natural way to avoid that line break with CSS without this line?
| var sub = stream_edit.get_sub_for_target(this); | ||
| edit_area.text(sub.description); | ||
| } | ||
|
|
There was a problem hiding this comment.
Hmm, ideally we'd avoid adding feature-specific code in this generic library. How about we make this a hook function and extend the map thing at the top to have a dict rather than just a function inside?
E.g.
var map = {
".stream-description-editable": {
on_save: stream_edit.change_stream_description,
on_start: stream_edit.get_raw_description,
}
...
};
As a sidenote, a larger refactor for this component might want to include extracting this little sub-mobile to e.g. static/js/editable_section.js -- there's no need for it to be in click_handlers.js
There was a problem hiding this comment.
@timabbott, I'm not too sure of what you're suggesting I do here. For the time being, I've added an extra (temporary) commit to this PR, based on what I think you've asked for me to do. If this isn't what you suggested (which I strongly suspect is the case), could you please clarify what exactly you're suggesting (regarding the hook function and extension of the map object)?
There was a problem hiding this comment.
Yes, this is what I'd had in mind! The benefit of this map model in general is that our generate content-editable component doesn't end up cluttered with individual bits of feature-specific conditionals, which makes it easier to reuse it for future features (or replace it with some other library, if we ever want to go that route).
|
I went ahead and merged this after squashing the last 2 commits, since this change now works well and we should make it available to users. We should investigate doing something better for stripping |
Purpose:
This PR completes delegating the responsibility of rendering of stream descriptions to the backend. Particularly, this PR contains 2 commits both of which are frontend changes which build upon the work done in PR #11284.
The first commit involves removing the existing system for rendering stream descriptions via. the marked.js library. Instead, we use the 'rendered_description' attribute now provided stream update and creation events.
The second commit is a UI change where when editing a stream description, instead of presenting the rendered HTML form of the text for the user to edit, the un-rendered form of the description is presented to be edited. This is a much needed change as before the when editing the stream description, all of the markdown data would be lost (see screenshots for a visual explanation).
Please read issue #11272, as well as the individual commit messages, for more details.
Testing Done:
Not many changes were required to be made to the automated frontend test suite. The main change here is the removal of the test for
stream_data.render_stream_description().GIFs or Screenshots:
After:
Before:
Additional Notes:
stream_data.get_sub_for_target()is now an exportable function.