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.

Sign up for GitHub

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

Jump to bottom

Render Stream Descriptions from the Backend - Frontend and UI Update #11480

Closed
wants to merge 3 commits into from
Closed

Render Stream Descriptions from the Backend - Frontend and UI Update #11480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
18d431f
stream_data: Use stream rendered_description provided by the backend.
Hypro999 Feb 5, 2019
62e67ff
click_handlers: Use stream desc. on edit instead of rendered desc.
Hypro999 Feb 7, 2019
7f00233
temp: Extend map in click_handlers.js
Hypro999 Feb 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 0 additions & 13 deletions frontend_tests/node_tests/stream_data.js
View file
Open in desktop
Expand Up @@ -570,19 +570,6 @@ run_test('remove_default_stream', () => {
assert.equal(page_params.realm_default_streams.length, 0);
});

run_test('render_stream_description', () => {
var desc = {
name: 'no_desc',
stream_id: 1002,
description: '<p>rendered desc</p>',
};

stream_data.add_sub('desc', desc);
var sub = stream_data.get_sub_by_name('desc');
stream_data.render_stream_description(sub);
assert.deepStrictEqual(sub.rendered_description, "rendered desc");
});

run_test('canonicalized_name', () => {
assert.deepStrictEqual(
stream_data.canonicalized_name('Stream_Bar'),
Expand Down
18 changes: 14 additions & 4 deletions static/js/click_handlers.js
View file
Open in desktop
Expand Up @@ -579,8 +579,14 @@ exports.initialize = function () {

(function () {
var map = {
".stream-description-editable": stream_edit.change_stream_description,
".stream-name-editable": stream_edit.change_stream_name,
".stream-description-editable": {
on_start: stream_edit.set_raw_description,
on_save: stream_edit.change_stream_description,
},
".stream-name-editable": {
on_start: null,
on_save: stream_edit.change_stream_name,
},
};

$(document).on("keydown", ".editable-section", function (e) {
Expand Down Expand Up @@ -626,6 +632,10 @@ exports.initialize = function () {
edit_area.attr("data-prev-text", edit_area.text().trim())
.attr("contenteditable", true);

if (map[selector].on_start) {
map[selector].on_start(this, edit_area);
}

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@Hypro999 Hypro999 Feb 9, 2019 •
edited

Choose a reason for hiding this comment

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

@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)?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

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).

ui_util.place_caret_at_end(edit_area[0]);

$(this).html("&times;");
Expand All @@ -634,8 +644,8 @@ exports.initialize = function () {

$("body").on("click", "[data-finish-editing]", function (e) {
var selector = $(this).attr("data-finish-editing");
if (map[selector]) {
map[selector](e);
if (map[selector].on_save) {
map[selector].on_save(e);
$(this).hide();
$(this).parent().find(selector).attr("contenteditable", false);
$("[data-make-editable='" + selector + "']").html("");
Expand Down
3 changes: 2 additions & 1 deletion static/js/server_events_dispatch.js
View file
Open in desktop
Expand Up @@ -247,7 +247,8 @@ exports.dispatch_normal_event = function dispatch_normal_event(event) {
stream_events.update_property(
event.stream_id,
event.property,
event.value
event.value,
event.rendered_description
);
settings_streams.update_default_streams_table();
} else if (event.op === 'create') {
Expand Down
11 changes: 4 additions & 7 deletions static/js/stream_data.js
View file
Open in desktop
Expand Up @@ -242,12 +242,6 @@ exports.get_subscriber_count = function (stream_name) {
return sub.subscribers.num_items();
};

exports.render_stream_description = function (sub) {
if (sub.description !== undefined) {
sub.rendered_description = marked(sub.description).replace('<p>', '').replace('</p>', '');
}
};

exports.update_calculated_fields = function (sub) {
sub.is_admin = page_params.is_admin;
// Admin can change any stream's name & description either stream is public or
Expand All @@ -268,7 +262,9 @@ exports.update_calculated_fields = function (sub) {
!sub.invite_only;
sub.preview_url = hash_util.by_stream_uri(sub.stream_id);
sub.can_add_subscribers = !page_params.is_guest && (!sub.invite_only || sub.subscribed);
exports.render_stream_description(sub);
if (sub.rendered_description !== undefined) {
sub.rendered_description = sub.rendered_description.replace('<p>', '').replace('</p>', '');
}
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

Without stripping the <p>, </p> tags, the edit symbol/button would get moved to the next line.
Like so:

Which is kinda ugly.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

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?

exports.update_subscribers_count(sub);
};

Expand Down Expand Up @@ -499,6 +495,7 @@ exports.create_sub_from_server_data = function (stream_name, attrs) {
push_notifications: page_params.enable_stream_push_notifications,
email_notifications: page_params.enable_stream_email_notifications,
description: '',
rendered_description: '',
});

exports.set_subscribers(sub, subscriber_user_ids);
Expand Down
11 changes: 11 additions & 0 deletions static/js/stream_edit.js
View file
Open in desktop
Expand Up @@ -149,6 +149,7 @@ function get_stream_id(target) {
return target.closest(".stream-row, .subscription_settings").attr("data-stream-id");
}


function get_sub_for_target(target) {
var stream_id = get_stream_id(target);
if (!stream_id) {
Expand Down Expand Up @@ -459,6 +460,16 @@ exports.change_stream_name = function (e) {
});
};

exports.set_raw_description = function (target, destination) {
var sub_settings = $(target).closest('.subscription_settings');
var sub = get_sub_for_target(sub_settings);
if (!sub) {
blueslip.error('set_raw_description() fails');
return;
}
destination.text(sub.description);
};

exports.change_stream_description = function (e) {
e.preventDefault();

Expand Down
4 changes: 2 additions & 2 deletions static/js/stream_events.js
View file
Open in desktop
Expand Up @@ -32,7 +32,7 @@ function update_stream_pin(sub, value) {
sub.pin_to_top = value;
}

exports.update_property = function (stream_id, property, value) {
exports.update_property = function (stream_id, property, value, rendered_description) {
var sub = stream_data.get_sub_by_id(stream_id);
if (sub === undefined) {
// This isn't a stream we know about, so ignore it.
Expand Down Expand Up @@ -65,7 +65,7 @@ exports.update_property = function (stream_id, property, value) {
subs.update_stream_name(sub, value);
break;
case 'description':
subs.update_stream_description(sub, value);
subs.update_stream_description(sub, value, rendered_description);
break;
case 'email_address':
sub.email_address = value;
Expand Down
4 changes: 2 additions & 2 deletions static/js/subs.js
View file
Open in desktop
Expand Up @@ -123,12 +123,12 @@ exports.update_stream_name = function (sub, new_name) {
message_live_update.update_stream_name(stream_id, new_name);
};

exports.update_stream_description = function (sub, description) {
exports.update_stream_description = function (sub, description, rendered_description) {
sub.description = description;
sub.rendered_description = rendered_description.replace('<p>', '').replace('</p>', '');

// Update stream row
var sub_row = row_for_stream_id(sub.stream_id);
stream_data.render_stream_description(sub);
sub_row.find(".description").html(sub.rendered_description);

// Update stream settings
Expand Down