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

Forum quickreply templatization #5414

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Forum quickreply templatization #5414

wants to merge 12 commits into from
+46 −12

Conversation

rica-carv
Copy link
Contributor

Motivation and Context

Enable custom template for the forum quick reply form

Description

Changed code to take hardcoded HTML from forum quickreply shortcode to template

How Has This Been Tested?

Latest e107 github code

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist

@codeclimate CodeClimate
Copy link

codeclimate bot commented Jan 28, 2025

Code Climate has analyzed commit 90ce61e and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Bug Risk 1

The test coverage on the diff in this pull request is 7.1% (80% is the threshold).

This pull request will bring the total coverage in the repository to 39.0% (0.0% change).

View more on Code Climate.

@CaMer0n
Copy link
Member

CaMer0n commented Jan 29, 2025 •
edited
Loading

@rica-carv Thank you for this! I left some comments. Just some minor tweaks and then I can commit it. Thank you.
Basically the form fields should all be shortcodes (other than 'form'). In other words, a bad template should not be able to break the functionality of the form.

@rica-carv
Copy link
Contributor Author

I left some comments.

@CaMer0n Sorry, but i don't see any coments.... anywhere....

@CaMer0n CaMer0n self-requested a review January 29, 2025 20:10
CaMer0n
CaMer0n reviewed Jan 29, 2025
View reviewed changes
e107_plugins/forum/templates/forum_viewtopic_template.php Outdated
<textarea cols='80' placeholder='{LAN=FORUM_2007}' rows='4' id='forum-quickreply-text' class='tbox input-xxlarge form-control' name='post' onselect='storeCaret(this);' onclick='storeCaret(this);' onkeyup='storeCaret(this);'></textarea>
</div>
<div class='center text-center form-group'>
<input type='submit' data-token='{QR_TOKEN}' data-forum-insert='{QR_AJAX}' data-forum-post='{QR_THFORUMID}' data-forum-thread='{QR_THID}' data-forum-action='quickreply' name='reply' value='{LAN=FORUM_2007}' class='btn btn-success button' />
Copy link
Member

Choose a reason for hiding this comment

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

The button and hidden element should be shortcodes.

Copy link
Contributor Author

@rica-carv rica-carv Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@CaMer0n Done

Just find a bit of a "overkill" to have a shortcode to output only the hidden field...
I couldn't manage it to do it with a var and without the sc definition....

e107_plugins/forum/templates/forum_viewtopic_template.php Outdated
@@ -481,6 +481,17 @@

";

$FORUM_VIEWTOPIC_TEMPLATE['quickreply'] = "
<form action='{QR_URL}' method='post'>
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the form tags from the template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just by curiosity, why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CaMer0n Done

@CaMer0n
Copy link
Member

CaMer0n commented Jan 29, 2025

@rica-carv Sorry, It appears they went into a 'pending' state. Do you see them now?

@rica-carv
Copy link
Contributor Author

Do you see them now?

@CaMer0n Yes i do. Tx

@rica-carv
Copy link
Contributor Author

Done.
Added default template for legacy purposes, althought it's akward to remove embbebed HTML asd then left it there for legacy purposes....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CaMer0n CaMer0n CaMer0n left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rica-carv @CaMer0n