Installer/upgrader templates revision - part 1 #4791
Conversation
Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
Signed-off-by: Gwenwyfar <gwenwyfar@simplemachines.org>
|
And that's most of all formatting for upgrader done for. The remaining for installer will come later. |
| <meta charset="', isset($txt['lang_character_set']) ? $txt['lang_character_set'] : 'UTF-8', '"> | ||
| <meta name="robots" content="noindex"> | ||
| <title>', $txt['smf_installer'], '</title> | ||
| <link rel="stylesheet" href="Themes/default/css/index.css?alp21"> |
There was a problem hiding this comment.
?alp21 needs to be dynamic check loadCSSFile function (same applies the next line as well).
| @@ -2127,30 +2129,32 @@ function template_chmod_files() | |||
| <td width="26%" valign="top" class="textbox"><label for="ftp_server">', $txt['ftp_server'], ':</label></td> | |||
| <td> | |||
| <div style="float: ', $txt['lang_rtl'] == false ? 'right' : 'left', '; margin-', $txt['lang_rtl'] == false ? 'right' : 'left', ': 1px;"><label for="ftp_port" class="textbox"><strong>', $txt['ftp_port'], ': </strong></label> <input type="text" size="3" name="ftp_port" id="ftp_port" value="', $incontext['ftp']['port'], '" /></div> | |||
| <input type="text" size="30" name="ftp_server" id="ftp_server" value="', $incontext['ftp']['server'], '" style="width: 70%;" /> | |||
| <input type="text" size="30" name="ftp_server" id="ftp_server" value="', $incontext['ftp']['server'], '" style="width: 70%;"> | |||
There was a problem hiding this comment.
Inline width should be moved to install.css (ignoring the following input's inline style).
| @@ -2202,7 +2206,7 @@ function template_database_settings() | |||
| <tr id="db_server_contain"> | |||
| <td width="20%" valign="top" class="textbox"><label for="db_server_input">', $txt['db_settings_server'], ':</label></td> | |||
| <td> | |||
| <input type="text" name="db_server" id="db_server_input" value="', $incontext['db']['server'], '" size="30" /><br> | |||
| <input type="text" name="db_server" id="db_server_input" value="', $incontext['db']['server'], '" size="30"><br> | |||
There was a problem hiding this comment.
Input sizes should be consistent and predefined for all (ignoring rest size issues).
| @@ -2378,15 +2382,15 @@ function template_populate_database() | |||
| if (!empty($incontext['failures'])) | |||
| { | |||
| echo ' | |||
| <div style="color: red;">', $txt['error_db_queries'], '</div> | |||
| <ul>'; | |||
| <div style="color: red;">', $txt['error_db_queries'], '</div> | |||
There was a problem hiding this comment.
All main colors should be predefined in index.css, if not should be added (ignoring rest inline-coloring).
| @@ -76,7 +76,7 @@ | |||
|
|
|||
| // The helper is crucial. Include it first thing. | |||
| if (!file_exists($upgrade_path . '/upgrade-helper.php')) | |||
| die('upgrade-helper.php not found where it was expected: ' . $upgrade_path . '/upgrade-helper.php! Make sure you have uploaded ALL files from the upgrade package. The upgrader cannot continue.'); | |||
| die('upgrade-helper.php not found where it was expected: ' . $upgrade_path . '/upgrade-helper.php! Make sure you have uploaded ALL files from the upgrade package. The upgrader cannot continue.'); | |||
There was a problem hiding this comment.
Why did you added extra tab here? I think its unnecessary
There was a problem hiding this comment.
Look more closely ;)
| ******************************************************************************/ | ||
| /* ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| Templates are below this point | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ */ |
There was a problem hiding this comment.
You should follow SMF's comment standard in here. While I understand the reasoning consistency should be fixed.
There was a problem hiding this comment.
If we are going by overall consistency the whole section is not consistent, and it is also non-existent in installer. Maybe removing it is a better choice, or leave an inline comment on both files.
| <div class="smalltext" style="padding: 5px; text-align: center;"><br>', $txt['upgrade_time_elapsed'], ': | ||
| <span id="mins_elapsed">', $mins, '</span> ', $txt['upgrade_time_mins'], ', <span id="secs_elapsed">', $seconds, '</span> ', $txt['upgrade_time_secs'], '. | ||
| </div>'; | ||
| <br> <br> <br> <br> <br> |
There was a problem hiding this comment.
I think this should be converted to margin instead of many following br's.
|
|
||
| if (!empty($upcontext['continue'])) | ||
| echo ' | ||
| <input type="submit" id="contbutt" name="contbutt" value="', $txt['upgrade_continue'], '"', $upcontext['continue'] == 2 ? ' disabled' : '', ' class="button">'; | ||
| <input type="submit" id="contbutt" name="contbutt" value="', $txt['upgrade_continue'], '"', $upcontext['continue'] == 2 ? ' disabled' : '', ' class="button">'; |
There was a problem hiding this comment.
'name' values should be removed as they are obsolete.
|
I'm not adding any new changes to this PR. It is what it is: mostly formatting. I'll have some of those suggestions in mind for the next one(s) :) |
|
PR was tested and I don't see anything broken and the PR is ok. @Gwenwyfar Will you be able to fix up the remaining issues Antes mentioned in a future PR? |
|
Yes, I already have some changes in local for part 2. Just splitting them so we don't have a giant PR to review. |
Getting started on these two, the templates here will need more work than the other ones on the html level. This is just mild formatting, next up will be removing the inline styles/table layout. This revision will also make both adaptive.