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

Compatible with wordpress version 6 #4

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

Conversation

desbest
Copy link

@desbest desbest commented Apr 18, 2023

Thank you for making this theme as well.

Copy link
Collaborator

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Dropped some general feedback for you; no tests or anything have been done. I don't currently run a WordPress site any more.

Hoping to see if @andreiluca will chime in.

Comment on lines -10 to +11
$comments_nr = lightword_fb_get_comment_type_count('comment');
$trackbacks_nr = lightword_fb_get_comment_type_count('pings');
// $comments_nr = lightword_fb_get_comment_type_count('comment');
// $trackbacks_nr = lightword_fb_get_comment_type_count('pings');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless you're planning to bring these features back, it's better to remove the lines entirely instead of commenting them out.

$oddcomment = 'alt ';
?>
<div id="tabsContainer">
<a href="#" class="tabs selected"><span><?php _e('Comments','lightword'); ?> (<?php echo $comments_nr; ?>)</span></a>
<a href="#" class="tabs selected"><span><?php _e('Comments','lightword'); ?> (<?php echo get_comments_number($post->ID); ?>)</span></a>
<a href="#" class="tabs"><span><?php _e('Trackbacks','lightword'); ?> (<?php echo $trackbacks_nr; ?>)</span></a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The variable $trackbacks_nr is now undefined after its definition above was commented out.

@@ -21,7 +21,8 @@

// Add a way for the custom header to be styled in the admin panel that controls
// custom headers. See yourtheme_admin_header_style(), below.
add_custom_image_header( '', 'basic_admin_header_style' );
add_theme_support('custom-header'. 'basic_admin_header_style');
// add_custom_image_header( '', 'basic_admin_header_style' );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as elsewhere: If it's obsolete code, delete, don't comment out.

Comment on lines +25 to +32
/*
array( 'name' => __('Cuf&oacute;n settings', 'lightword'),
'desc' => __('Show certain text on your blog (blog title, tagline, post titles, page titles, etc.) using Cuf&oacute;n&sup1; or the lighter weight, more modern CSS3 <tt>font-face</tt> declaration.<br /><br />If using Cuf&oacute;n, select <em>Extra</em>&sup2; (or <em>Disabled</em>) for languages with accents and special characters.','lightword'),
'id' => 'lw_cufon_settings',
'options' => array(__('Enabled','lightword'), __('Disabled','lightword'), __('Extra','lightword'), __('CSS3 Font-face (lightweight)')),
'std' => __('Enabled','lightword'),
'type' => 'select'),
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete, don't comment out.

Comment on lines -403 to +405
lightword_fb_update_comment_type_cache($p);
//lightword_fb_update_comment_type_cache($p);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete, don't comment out.

@@ -127,7 +144,7 @@ img.wp-smiley{border:0px;vertical-align:middle;}
/* SIDEBAR */
.content-sidebar{width:191px;display:inline-block;overflow:hidden;vertical-align:top;}
.content-sidebar input{padding:3px;border:1px solid #E5E2E0;margin-bottom:2px;}
.content-sidebar h3{margin:8px 0 0 0 !important;display:block;background:#FFF url(images/sidebar_h3.png) no-repeat;height:22px;width:181px;font-weight:700;font-size:11px;padding:9px 0 0 10px;}
.content-sidebar h3{margin:8px 0 0 0 !important;display:table;background:#FFF url(images/sidebar_h3.png) no-repeat;height:22px;width:181px;font-weight:700;font-size:11px;padding:9px 0 0 10px;} /* desbest edit */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.content-sidebar h3{margin:8px 0 0 0 !important;display:table;background:#FFF url(images/sidebar_h3.png) no-repeat;height:22px;width:181px;font-weight:700;font-size:11px;padding:9px 0 0 10px;} /* desbest edit */
.content-sidebar h3{margin:8px 0 0 0 !important;display:table;background:#FFF url(images/sidebar_h3.png) no-repeat;height:22px;width:181px;font-weight:700;font-size:11px;padding:9px 0 0 10px;}

@@ -226,3 +243,99 @@ ol.snap_nav{list-style:none;display:block;}
ol.snap_nav li{display:inline; background-color:#F4F4F4;padding:3px;}
ol.snap_nav .snap_selected{background-color:#DDD;}
.simple_date{background:url(images/date.png) 5px 50% no-repeat;padding:8px 25px;background-color:#EEE;margin-bottom:1em;}

/* desbest edit */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question as above re: whether this is customization or part of the patch.

box-sizing: border-box !important;
overflow-x: clip;
}
/*.content-sidebar { max-width: 163px; }*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/*.content-sidebar { max-width: 163px; }*/

Comment on lines +269 to +272
/*max-width: 160px;*/
background-color: #fcfcfc;
/*background-image: none;*/
/*border: 1px solid #dcdcdc;*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/*max-width: 160px;*/
background-color: #fcfcfc;
/*background-image: none;*/
/*border: 1px solid #dcdcdc;*/
background-color: #fcfcfc;

.content-sidebar { width: 75%; max-width: unset; }
.content-sidebar h3 { background-size: 100%; }
.content-sidebar #searchform { margin-bottom: 4em; }
#searchform { /* background-image: none !important; */ height: unset; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#searchform { /* background-image: none !important; */ height: unset; }
#searchform { height: unset; }

@desbest
Copy link
Author

desbest commented Apr 21, 2023

Empty array needs further examination

$post_id_list[] = '';
If you resurrect this function instead of deleting it completely, this change seems very likely to change behavior in an unintended way.

I forgot that was even there. And it's not even being used. It was enough to cause a fatal error that stopped the theme from displaying, before I made any edits to it.

Whatever that array and line of code is for, I'll look into it, later on.

Cufon Fonts and Facebook Sharing

//lightword_fb_update_comment_type_cache($p);

Yes I should remove the Cufon code from the theme instead of commenting it out. The same applies for the facebook sharing code. I forgot to remove it. Yes why is it still in there?

PS. Nobody is going to be using facebook widgets like share, comments, likes, to be embedded onto their website because Facebook did a bait and switch back in 2014 by cutting the organic reach of pages and groups, to encourage people to pay to boost their posts, which put a complete end to the era of free organic traffic. The companies that had paid thousands of dollars, for likes on their page for the promise of perpetual free traffic, were heavily scammed.

Makeshift customisation features

add_custom_image_header()

Back when the theme was made in 2008, there was no standardised way to allow webmasters with zero HTML and CSS knowledge, to customise their theme with commonly requested changes, like changing the background, font, colours and logo. Now there is.

I'll look into that line that's been commented out, to see if there is currently a native theme customiser replacement and whether I can be bothered to add a native one in.

Mobile user check

 function isMobile() {
    var index = navigator.appVersion.indexOf("Mobile"); // detect chrome for android and ios safari
    var index2 = navigator.appVersion.indexOf("Android"); // detect firefox for android
    if (index2) { return (index2 > -1); }
    return (index > -1);
    }   

Clearer variable names would make this function a lot easier to follow.

The function in question is only 4 lines of code and as evident by the comments I've included besides it, it should be obvious from a cursory glance, exactly what the javascript function is designed to do.
It's designed to detect whether the user is using a smartphone to access the website or not, by checking the user agent.

Footer credits

@andreiluca Thoughts? I couldn't readily find a page about LightWord or WP themes on your site just now.

Finders keepers, losers weepers! Don't neglect or abuse it, use it or lose it. 😜


I agree with everything you've suggested, except for the things you've commented that I'll be addressing in my third response.

@desbest
Copy link
Author

desbest commented Apr 21, 2023

Pingbacks and trackbacks (and maybe webmentions)

<!-- <link rel="pingback" href="<?php //bloginfo('pingback_url'); ?>" /> -->

Broken record says: If it's dead code, it's dead code; remove it.

The world of pingbacks and trackbacks is a very different place in 2023 than it was in 2008. I need to find out in retrospect, whether trackbacks managed to sufficently solve the spam problem in a way that the predecessor pingbacks did not.

About the trackback issue, I need to do some more research before deciding a good decision to make on this.

// $comments_nr = lightword_fb_get_comment_type_count('comment');
// $trackbacks_nr = lightword_fb_get_comment_type_count('pings');
a href="#" class="tabs"><span><?php _e('Trackbacks','lightword'); ?> (<?php echo $trackbacks_nr; ?>)</span></a>

The variable $trackbacks_nr is now undefined after its definition above was commented out.

Seeing as trackbacks is the spiritual successor of pingbacks, designed to prevent most of the spam that plagued pingbacks, when people have made webmention, that unfortunately attracted much less attention and general knowledge than it did for webmentions, I now need to check whether webmention manages to solve any of the spam issue for pings, that trackback promised to solve, whether it succeeds whether trackbacks failed from bad implementation to a foreseen practical mechanism or if their counter-measure successfully solved the forseen practical adverse scenario as intended but couldn't solve the unforseen one that wasn't predicted.

I'll research how spam worthy the trackbacks and webmentions are, later this month.

I'll look into it later.

@desbest
Copy link
Author

desbest commented Jun 19, 2023

The readme

Would be better to convert the old readme to Markdown, IMO. The new readme.md is pretty sparse.

I see your point but nobody wants to see a readme that includes a changelog that dates back to 2008 with a huge list of minimal changes that would have no relevance now, as the theme has pretty much matured beyond its numerous incremental changes. It's stable now.

Custom theming options

add_custom_image_header( '', 'basic_admin_header_style' );
add_theme_support('custom-header'. 'basic_admin_header_style');

Same as elsewhere: If it's obsolete code, delete, don't comment out.

That was a reminder for me to later analyse the built-in theme options, not all the functionality from the makeshift customisation has been moved over to the native customisation.

I'll look into it later.

Comments for debugging the template tags

// var_dump(wp_count_comments($post->id)->approved); exit();

Another commented-out line that should be just removed.

This line is there for a reason, as the comments section needs to be improved as it's unfinished. I need to test out and fix the threaded comments feature.

I'll look into it later.

@desbest
Copy link
Author

desbest commented Jun 20, 2023

Responsive design

/*.content-sidebar  { max-width: 163px; }*/
/*max-width: 160px;*/
background-color: #fcfcfc;
/*background-image: none;*/
/*border: 1px solid #dcdcdc;*/
#searchform { /* background-image: none !important; */ height: unset; }
/* DESBEST EDIT */
.content-sidebar #searchform, #content-body #searchform { margin-bottom: 1em; }
.content-sidebar #searchform input[type="text"], #content-body #searchform input[type="text"] { margin-top: 2px; padding-left: 8px; width: 11em; height: 1.15em; background: none; border: 0px; }
.content-sidebar #searchform input[type="submit"], #content-body #searchform input[type="submit"] { width: 2.5em; opacity: 0; }
.screen-reader-text { display: none; }

Is this a customization or part of the changes?

Customization -> remove from patch
Part of changes > needs a better section header, and/or to be integrated into the relevant sections elsewhere

To use your words, I would say that it's part of changes but it has been kept at the bottom of the stylesheet as a new rule instead of being appended to the existing rule, for a reason.

I have a code policy for always including all code regarding responsive design, to be put at the bottom of the stylesheet, even if it exists outside a media query (of a 700px small screen width).

The design had visual glitches on the smartphones so I added in some new rules so it'll look better on mobile. Sure some of those rules also apply on the desktop as well but I kept it anyway as it made things look nicer.

The authorship comments and comments for non-executing code, is also there for a reason

/* display:inline;float:right; */

display: block; text-align: right; color: #c0c0c0;

/* desbest edit */

Irrelevant comments. Dead code should be removed, and we don't need line-by-line authorship credits.

And again....

.content-sidebar { width:  75%; max-width: unset; }
.content-sidebar h3 { background-size: 100%; }
.content-sidebar #searchform { margin-bottom: 4em; }
#searchform { /* background-image: none !important; */ height: unset; }
/*max-width: 160px;*/
background-color: #fcfcfc;
/*background-image: none;*/
/*border: 1px solid #dcdcdc;*/
box-sizing: border-box !important;
overflow-x: clip;
}
/*.content-sidebar  { max-width: 163px; }*/
ol.snap_nav{list-style:none;display:block;}
ol.snap_nav li{display:inline; background-color:#F4F4F4;padding:3px;}
ol.snap_nav .snap_selected{background-color:#DDD;}
.simple_date{background:url(images/date.png) 5px 50% no-repeat;padding:8px 25px;background-color:#EEE;margin-bottom:1em;}
/* desbest edit */
/* SIDEBAR */
.content-sidebar{width:191px;display:inline-block;overflow:hidden;vertical-align:top;}
.content-sidebar input{padding:3px;border:1px solid #E5E2E0;margin-bottom:2px;}
.content-sidebar h3{margin:8px 0 0 0 !important;display:block;background:#FFF url(images/sidebar_h3.png) no-repeat;height:22px;width:181px;font-weight:700;font-size:11px;padding:9px 0 0 10px;}
.content-sidebar h3{margin:8px 0 0 0 !important;display:table;background:#FFF url(images/sidebar_h3.png) no-repeat;height:22px;width:181px;font-weight:700;font-size:11px;padding:9px 0 0 10px;} /* desbest edit */
/* display:inline;float:right; */
display: block; text-align: right; color: #c0c0c0;
/* desbest edit */

My response to this is that the comments are there for a reason

  • Visual glitches
  • Visual quirks
  • Rendering engine inconsistencies
  • Box model wrangling (pre-redesign)
  • Newfound child elements disrupting the box model (post-redesign)
  • Elements not being at the perfect size, position, margin or internal alignment.

It would take a long time for me to sufficiently explain what they are but it's worthy of a blog article.

To keep it short, websites do not perfectly look the same in every web browser, sometimes the design falls short of what the designer had intended due to technological limitations and sometimes inserting one element or making a slight tweak to one element can end up disrupting the layout of the page.

The comments are designed to help mitigate and solve issues like these, instead of having any future web designer, be spending hours wondering why one small thing doesn't work, spending 8+ hours frustratingly working from trial and error by making endless incremental changes

@desbest
Copy link
Author

desbest commented Jun 20, 2023

That's the end of all my responses to your code review.
Thank you for reviewing 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.

2 participants