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

Post Templates #1151

Open
wants to merge 28 commits into
base: develop
Choose a base branch
from
Open

Post Templates #1151

wants to merge 28 commits into from

Conversation

MoshiKoi
Copy link
Member

@MoshiKoi MoshiKoi commented Aug 2, 2023

Proof of concept that I made in an afternoon lol

Addresses https://meta.codidact.com/posts/276900

Templates are tied to a post type, so you can have templates for questions, answers, articles, help docs, and even templates for templates (probably not useful, but hey, why not).

Database changes:

  • Adds a new field to posts, template_post_type_id
  • Adds before_template_post_type_id and after_template_post_type_id to post_histories

Seed changes

  • Creates a new PostType, PostTemplate
  • Seeds a new default Category called Templates to create and edit templates. The default permission level is that everyone can view, but only moderators can edit
  • Due to behavior changes (below), the sequence field of Q&A and Meta are now seeded as 1 and 2, respectively

Behavior changes

  • Settings the "sequence" of a category to -1 will now hide the category.

Other

Trust Level only affects creating posts afaik, so users with the edit ability will still be able to edit these posts since by default, I let everyone be able to view the category. This is easily configured since it's just a normal category, so if you like, you can set the permissions however you want, and even lower it to let normal users create templates.

app/assets/javascripts/posts.js Outdated Show resolved Hide resolved
app/models/post_type.rb Outdated Show resolved Hide resolved
@ArtOfCode-
Copy link
Member

More generally: I'm not sure that having a Templates category at the top of every page on the site is right - it almost feels too visible for something that won't be used that frequently. I like having it as a category, because it makes template admin familiar, but can we find a way to hide the Templates category from the top bar? Might need to add a field to the Category model. It'll still be linked from /categories, just not on every page.

@MoshiKoi
Copy link
Member Author

MoshiKoi commented Aug 6, 2023

You can now remove the category from the header by setting the sequence setting to null. Seeds have also been updated.

@MoshiKoi MoshiKoi marked this pull request as ready for review September 7, 2023 15:39
@MoshiKoi
Copy link
Member Author

MoshiKoi commented Sep 7, 2023

In terms of functionality, everything should be there, but the UI is really ugly; if anyone has any ideas how to improve it, please let me know.

image

Templates are a markdown tool which lists each template for the post type; clicking on the name will fill in the template, clicking on the info button will show the second modal with information about the template, including a link to the template post.

@cellio
Copy link
Member

cellio commented Sep 7, 2023

In terms of functionality, everything should be there, but the UI is really ugly; if anyone has any ideas how to improve it, please let me know.

Can you position the modal over the post editor (buttons + textbox), read-only with the "apply" action being more obvious?

What happens to text that's already in the editor when you apply a template? Does the template get inserted after (or before), are templates not available if there's text, or does existing text get clobbered? (Any but the last is ok with me, preferring an additive approach over disabling.)

@MoshiKoi
Copy link
Member Author

MoshiKoi commented Sep 8, 2023

Template content is inserted, not replaced (though apparently I forgot to push that change before going on my break from QPixel, whoops)

@cellio cellio requested a review from a team September 8, 2023 23:26
@cellio
Copy link
Member

cellio commented Sep 12, 2023

You can now remove the category from the header by setting the sequence setting to null. Seeds have also been updated.

Some existing categories don't set a value. Consider -1 or even 0 as opposed to null?

@MoshiKoi
Copy link
Member Author

You can now remove the category from the header by setting the sequence setting to null. Seeds have also been updated.

Some existing categories don't set a value. Consider -1 or even 0 as opposed to null?

Was allowing existing categories to not have a value intentional behavior before this PR? If so, what was the expected behavior? I had assumed that categories not being seeded with values was a bug.

@cellio
Copy link
Member

cellio commented Sep 26, 2023

I believe the default is to show them in order of creation, and until a community needs something beyond Q&A and Meta, there was no reason to set explicit values. (Even then, IIRC correctly, some just set Meta to 99 and otherwise leave them blank.) For intent we'll have to ask @ArtOfCode- ; I'm just reporting what's in production. I don't have an objection to requiring categories to specify where they are in the sequence, but we'll need to add migrations for ones that don't have it set today.

@ArtOfCode-
Copy link
Member

It was intentional to allow categories to not set the sequence value, if that helps - empty has so far been a valid value.

@MoshiKoi
Copy link
Member Author

MoshiKoi commented Oct 4, 2023

Ah, OK. If having it blank had well defined behavior, I'm fine with using -1 instead

@Taeir
Copy link
Contributor

Taeir commented Nov 11, 2023

It seems impossible to select the templates post type as listed for a category since the old value remains in the redis cache forever. This may be a problem as editing the "Templates" category might clear templates from the listed values, as it was not a selectable value.

Perhaps we need to do something with seeds or after_create handlers to clear specific cache values and make things more robust to changes. Of course, post_types almost never change, but having behavior in the code to automatically deal with necessary cache invalidation for it should a new one be created would be useful. Not really something for this PR though.

@Taeir
Copy link
Contributor

Taeir commented Nov 11, 2023

Since the title name of the template is important (it is shown to the user in the selection dropdown), it could be common to have duplicate names for different post types, e.g. "Default" both for Question and for Answer. However, after creation you cannot see in the list what post type it was for.

I suggest to add to /app/views/posts/_list.html.erb, line 36:

        <%= post.post_type.is_top_level ? post.title : post.parent.title %>
        <%= post.post_type.is_closeable && post.closed && !post.duplicate_post  ? "[closed]" : "" %>
        <%= post.duplicate_post && post.post_type.is_closeable && post.closed ? "[duplicate]" : "" %>
+       <%= post.post_type.name == 'PostTemplate' ? "[#{post.template_post_type.name}]" : "" %>
      <% end %>
    </div>
    <% if (SiteSetting['PostBodyListTruncateLength'] || 0) > 0 %>

Which will show up as follows in the list:

image

app/views/shared/_body_field.html.erb Outdated Show resolved Hide resolved
db/seeds/categories.yml Show resolved Hide resolved
db/seeds/categories.yml Show resolved Hide resolved
const $postField = $('.js-post-field');
$tgt = $(ev.target);
const content = $(`#template-md-${$tgt.attr('data-template-id')}`).val();
replaceSelection($postField, content);
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it is intentional that templates are overwriting the selection, or if nothing is selected, just append at the cursor location?

I'm more used to implementations where templates replace the current content. What was the idea behind the different approach? To allow defining sort of common elements as templates?

I can see it happening that a user tries to select text and then accidentally clicks on a template, after which their selection is gone (no confirmation). Not sure if that weighs up against the extra effort of having to confirm something.

Copy link
Member Author

Choose a reason for hiding this comment

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

See Monica's comment here: #1151 (comment), but also a confirmation would also be nice to have if they still have selected text that might be overridden (undo functionality is broken on all Markdown tools (#1152), so there's no going back).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah in my tests if you select text it does override (which is luckily consistent with what the javascript method is named after). The question is how complex we would want to make it.

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.

5 participants