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 1 commit
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
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
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