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

Remove Hardcoded HTML #756

Closed
Starpaul20 opened this issue Jun 16, 2014 · 28 comments
Closed

Remove Hardcoded HTML #756

Starpaul20 opened this issue Jun 16, 2014 · 28 comments
Assignees
Labels
b:1.8 Branch: 1.8.x s:resolved Status: Resolved. Solution implemented or scheduled t:enhancement Type: Enhancement. Contains minor improvements
Milestone

Comments

@Starpaul20
Copy link
Member

All hardcoded HTML should be moved to templates.

@Starpaul20 Starpaul20 added this to the 1.8 Beta 2 milestone Jun 16, 2014
@Destroy666x
Copy link
Contributor

Wanted to ask in development forum but since the issue is already here and non-staff may want to know it too - to what extent should it be done? For example I'm sure codes like this from inc/functions_user.php need to be moved:

$folderlinks .= "<div><a href=\"private.php?fid=$folderinfo[0]\" class=\"usercp_nav_item {$class}\">$folderinfo[1]</a></div>\n";

However, what about all short one-liners? I think such should be moved as they affect the design:

$lastpost = "<div style=\"text-align: center;\">-</div>";

but there are also a lot like this (especially in modcp.php):

$subject = "<strong>{$lang->ipresult_lastip}</strong> {$profile_link}";

with only one not-so-relevant tag like strong, a or option. Should they be moved too and won't that create too many templates?

@ATofighi
Copy link
Contributor

  • calendar.php: lines 413, 417, 421, 425, 429, 433, 444, 448, 452, 456, 460, 464, 500, 504, 1009, 1013, 1017, 1021, 1025, 1029, 1040, 1044, 1048, 1052, 1056, 1060, ...
  • editpost.php line: 790
  • forumdisplay.php lines: 240, 1240
  • global.php lines: 295, 299, 670, 688, 692, 715, 754
  • inc/functions.php lines: 1736, 1741, 1747, 2948, 2959, 2967, 3119, 3129, 3138, 3148, 3151, 3274, 3277, 3292, 3312, 3316, 3320, 3518, 3532, 4373, 4444, 4445, 4454, 4455, 4456, 4509, 4525, 4528, 5239, 6090, 6121, 6123
  • inc/functions_calendar.php
  • inc/functions_forumlist.php
  • inc/functions_modcp.php
  • inc/functions_post.php
  • inc/functions_search.php
  • inc/functions_user.php
  • member.php
  • memberlist.php
  • misc.php
  • modcp.php
  • newreply.php
  • newthread.php
  • portal.php
  • printthread.php
  • private.php
  • reputation.php
  • search.php
  • showthread.php
  • usercp.php

should be change.

@JordanMussi
Copy link
Contributor

$folderlinks .= "<div><a href=\"private.php?fid=$folderinfo[0]\" class=\"usercp_nav_item {$class}\">$folderinfo[1]</a></div>\n";

This should be moved to a template realistically...

$lastpost = "<div style=\"text-align: center;\">-</div>";

To achieve a good looking theme. "Hacks" may be used to get around ugly things like this therefore they should be converted.

$subject = "<strong>{$lang->ipresult_lastip}</strong> {$profile_link}";

The <strong> tags could possibly go in the language string but that's not removing the hard coded HTML. Using the template and passing $profile_link to it would be good as the theme designer can add/remove the <strong> tags wrapped around {$lang->ipresult_lastip} in the template.


Personally since the goal was to remove all hard coded HTML, any HTML that isn't in a template (in the frontend) needs to be moved to a template.
All of the above is just my opinion and may not reflect the outcome of this ticket.

@Sama34
Copy link
Contributor

Sama34 commented Jun 17, 2014

In the las example there you can use the language system instead. Does $lastpost even needs to be not empty?

@Sama34
Copy link
Contributor

Sama34 commented Jun 17, 2014

This should also take care of JS files with hardcoded links, which will require more complex fixes.

@JordanMussi
Copy link
Contributor

In the las example there you can use the language system instead. Does $lastpost even needs to be not empty?

That's still hard coded, and something the theme author cannot change easily (i.e. they have to tell the user to change to change it). Therefore that should also be moved to a template.

@Eric-Jackson
Copy link
Contributor

Instead of using <strong></strong>, something along the lines of <span class="importantText"></span> would be more appropriate with font-weight: bold; set in CSS so they could be changes more globally rather than finding each <strong>. Regardless I do agree that even short ones like that should be moved into templates.

@ghost
Copy link

ghost commented Jun 19, 2014

Just to mention here specifically: 'Buddy / Ignore' buttons of Member Profile (in member.php) should be variable driven and that may call 4 additional templates. They are currently coded in a very rigid way ...

@Stefan-MyBB
Copy link
Contributor

There is also some HTML within the language files.

Starpaul20 added a commit that referenced this issue Jun 21, 2014
@Starpaul20
Copy link
Member Author

I've started to go through the files and removing hardcoded HTML. This commit removes most of the HTML from calendar.php and moves it to templates.

Starpaul20 added a commit that referenced this issue Jun 21, 2014
Fixed bug in calendar.php
Starpaul20 added a commit that referenced this issue Jun 21, 2014
Starpaul20 added a commit that referenced this issue Jun 21, 2014
global.php language option
Starpaul20 added a commit that referenced this issue Jun 21, 2014
member.php (profile avatar, website and reputation vote link)
Starpaul20 added a commit that referenced this issue Jun 21, 2014
misc.php (Syndication forumlist)
@Sama34
Copy link
Contributor

Sama34 commented Jun 21, 2014

@DEMONATE the buddy code can be easily moved to a single template, not need for four.

Starpaul20 added a commit that referenced this issue Jun 21, 2014
modcp.php part 1 (banning, mod logs and home page)
Starpaul20 added a commit that referenced this issue Jun 22, 2014
portal.php (announcement post icon)
Starpaul20 added a commit that referenced this issue Jun 22, 2014
printthread.php (multipage)
Starpaul20 added a commit that referenced this issue Jun 23, 2014
search.php (post icon) and showthread (quick reply mod options and similar threads post icon)
@DiogoParrinha
Copy link
Contributor

@PaulBender don't forget to use the task list created by @ATofighi so you know when you're done.
Also remember that as you've been committing directly here, this must be done in time for Beta 2.

Starpaul20 added a commit that referenced this issue Jun 24, 2014
reputation.php (adding rep delete button and drop down options)
Starpaul20 added a commit that referenced this issue Jun 24, 2014
functions_user.php (PM folders); cached some templates in reputation.php
Starpaul20 added a commit that referenced this issue Jun 24, 2014
functions_calendar.php (calendar jump)
Starpaul20 added a commit that referenced this issue Jun 24, 2014
modcp.php part 2 (announcements, mod queue, edit user, ip lookup)
Starpaul20 added a commit that referenced this issue Jun 24, 2014
functions_forumlist.php (moderators, last post, viewers, unapproved stuff)
Starpaul20 added a commit that referenced this issue Jun 26, 2014
usercp.php part 1 (date & time formats, post/threads per page, forum subscriptions and avatar auto resize)
Starpaul20 added a commit that referenced this issue Jun 27, 2014
usercp.php part 2 (no buddies/ignored, draft thread/forum links, group leaders)
@Destroy666x
Copy link
Contributor

They could indeed be moved to one template (2 images with src="{$theme['imgdir']}/{$add_remove_buddy}.png" or similar) but it won't be possible after this commit: 3479f64 which hides ignore list button if user is on buddy list and the other way round. So, yes, the buttons are already in separate templates.

Also, what will we do about HTML in parser (quotes etc.)? I see that parser file is not included in the list.

Starpaul20 added a commit that referenced this issue Jun 27, 2014
usercp.php part 3 (joinable groups and members of groups)
Starpaul20 added a commit that referenced this issue Jun 27, 2014
usercp.php part 4 (home page, post icons)
Starpaul20 added a commit that referenced this issue Jun 27, 2014
Fixed minor bug in usercp.php
@JordanMussi
Copy link
Contributor

Changing the parser could be somewhat problematic as we'd have to load several additional templates on most if not all showthread requests. I say it's possibly better to leave the parser be.

Starpaul20 added a commit that referenced this issue Jun 27, 2014
member.php (user star, multi option profile fields, COPPA day dropdown)
Starpaul20 added a commit that referenced this issue Jun 27, 2014
memberlist.php (user star)
@Starpaul20
Copy link
Member Author

Okay, this is done.

@DiogoParrinha
Copy link
Contributor

That was quite some work!

Starpaul20 added a commit that referenced this issue Jun 27, 2014
Missed two bits in usercp.php
@Starpaul20
Copy link
Member Author

Though all the commits above, I have added 127 templates and removed one (usercp_options_stylebit).

Starpaul20 added a commit that referenced this issue Jun 27, 2014
Put code in wrong place >_<
@DiogoParrinha
Copy link
Contributor

@PaulBender make sure not to forget to cache templates! I don't have time to check if you did.

@Starpaul20
Copy link
Member Author

I cached as I went. There shouldn't be any uncached templates.

@DiogoParrinha
Copy link
Contributor

Sounds good then :)

@DiogoParrinha
Copy link
Contributor

Closing this. Bugs will be reported during Beta 2 testing period.

@JordanMussi
Copy link
Contributor

https://github.com/mybb/mybb/pull/601/files#diff-3ab6fc25d9d5e6f0a2a9432c19b14717R35 has added HTML to the language file (not the template).

Also was #851 removed as well?

@xykul
Copy link

xykul commented Jul 4, 2014

Raise the thread or post approval
Go to index
functions_forumlist.php line 567 or 582 errors

eval("\$unapproved_posts = \"".$templates->get("forumbit_depth2_forum_unapproved_posts")."\";");
eval("\$unapproved_threads = \"".$templates->get("forumbit_depth2_forum_unapproved_threads")."\";");

or
AdminCP edit forum..
"Yes, moderate new posts" and "Yes, moderate new threads" click..
Whether members of this forum thread or send a message.
Administrators and moderators receive the above error.

Starpaul20 added a commit that referenced this issue Jul 4, 2014
Forgot to global $templates...
@JN-Jones
Copy link
Contributor

Starpaul20 added a commit that referenced this issue Jul 10, 2014
global.php (task image)
@Destroy666x
Copy link
Contributor

There are 2 more in global.php:

Also, what's up with display: show; in one of the last lines in the same file? Never heard of that value: http://www.w3.org/TR/CSS21/visuren.html#display-prop I can't find where _c is used, so I guess it's used for tbody like _e and thus should be display: table-row-group;

@ATofighi
Copy link
Contributor

ATofighi commented Aug 6, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
b:1.8 Branch: 1.8.x s:resolved Status: Resolved. Solution implemented or scheduled t:enhancement Type: Enhancement. Contains minor improvements
Projects
None yet
Development

No branches or pull requests

10 participants