updating packages #99

iruzevic · 2018-08-01T12:55:16Z

I have refactored a lot of stuff. Before you check this PR we should discus what I did and why also I will check rename script because there were a lot of changes in code.

…te into feature/refactoring

iruzevic · 2018-08-04T23:28:33Z

I have also updated readme file with all details how to use and run boilerplate

dingo-d · 2018-08-08T17:02:00Z

README.md


-We are using [Infinum coding standards for WordPress](https://github.com/infinum/coding-standards-wp) to check php files.
+    ```bash
+    npm run precommit


Can we setup githooks here instead of manually running scripts? Githooks

sure we can add that after we test it out. lets set this for version 3.1?

dingo-d · 2018-08-08T17:03:05Z

README.md


-* Add this aliases to you bash config:
+3. **Linting PHP** - If you have composer installed add this aliases to you bash config and reload terminal:


We should rephrase this to be: you can add these aliases to your bash/zsh config for easier usage.

dingo-d · 2018-08-08T17:04:49Z

README.md

+    bash bin/build.sh
+    ```
+
+3. When rsyncing to server use `ci-exclude.txt` to exclude unnecesery folders and files on server.


Add a phrase: 'For deployment, we are using rsync in ore continuous deployment setup', and maybe add a sample example.

…edia, and replacing normalize.css with sass version for lighter output, and fixed all terminal warnings

iruzevic · 2018-09-01T21:01:39Z

packages updated, fixed creation of source maps, minor bug in class-media, and replacing normalize.css with sass version for lighter output, and fixed all terminal warnings

please check end merge this. so we can release version 3 tnx @dingo-d

iruzevic · 2018-09-01T21:11:10Z

updating composer and fixing all linting errors and warnings

dingo-d · 2018-09-02T14:52:28Z

README.md


-If you are using VVV clone it in the `public_html` folder
+    ```bash


rename should be done either with shell script or using node script. In order for node script to work, npm install should be ran and then npm run rename iirc. This should be added to the readme.

you run sh bin/rename.sh and that runs

npm install npm run rename

this shell script should run on windows and osx. my idea is to run one command and you are done not 2 or more :D

I'll try this out on Windows 👍

dingo-d · 2018-09-02T14:54:32Z

README.md

+    ```bash
+    alias phpcs='vendor/bin/phpcs';
+    alias phpcbf='vendor/bin/phpcbf';
+    alias wpcs='phpcs --standard=vendor/infinum/coding-standards-wp/Infinum';


In the case where phpcs is run using the vendor version, the standard isn't necessary since it will pick up the phpcs.xml.dist as the configuration. In that file we have specified the Infinum's coding standard as a default. So the last two aliases are not really needed 😉

ok I will remove it then, great work

dingo-d · 2018-09-02T15:00:40Z

bin/rename.js

    console.error('Error occurred:', error);
  }
 };


 // Main script
-consoleOutput(fgGreen, 'The script will uniquely set up your theme.');
+consoleOutput(fgGreen, 'Welcome to Boilerplate readme script. The script will uniquely set up your theme.');


Welcome to Boilerplate readme script

You meant Welcome to Boilerplate rename script ? 😄

dingo-d · 2018-09-02T15:11:02Z

wp-content/themes/inf_theme/admin/class-admin.php

-    $main_style = $this->general_helper->get_manifest_assets_data( 'applicationAdmin.css' );
-    wp_register_style( static::THEME_NAME . '-style', $main_style );
+    $main_style = General_Helper::get_manifest_assets_data( 'applicationAdmin.css' );
+    wp_register_style( static::THEME_NAME . '-style', $main_style, array(), static::THEME_VERSION );


Wouldn't static::THEME_VERSION affect cache busting we are doing using manifest.json?

I know that there is an error that happens because theme version isn't specified, but there is an upstream issue opened at WPCS about this (they neglected the fact that hashing can be used).

You can either disable these lines with an inline comment

// phpcs:ignore WordPress.WP.EnqueuedResourceParameters.MissingVersion

Or by adding this in the phpcs.xml.dist ruleset and disabling it.

I have tested this and we are ok, assets manifest changes the name of the output file every time something changes so, everytime we hit build name of the file will change and cache busting will be done.

Awesome, I thought that specifying version would suppress it. 👍

dingo-d · 2018-09-02T15:13:58Z

wp-content/themes/inf_theme/admin/class-admin.php

-    $main_script = $this->general_helper->get_manifest_assets_data( 'applicationAdmin.js' );
-    wp_register_script( static::THEME_NAME . '-scripts', $main_script );
+    $main_script = General_Helper::get_manifest_assets_data( 'applicationAdmin.js' );
+    wp_register_script( static::THEME_NAME . '-scripts', $main_script, array(), static::THEME_VERSION ); // phpcs:ignore WordPress.WP.EnqueuedResourceParameters.NotInFooter


Why aren't we loading the scripts in the footer? I thought that it's better to load them all in the footer.

because this is up to the developer to decide when to load scripts we can load them in the footer in the boilerplate.

dingo-d · 2018-09-02T15:14:37Z

wp-content/themes/inf_theme/admin/class-admin.php

@@ -71,7 +49,7 @@ public function enqueue_scripts() {
   * @param string $color_scheme Color scheme string.
   * @return string              Modified color scheme.
   *
-   * @since 2.1.0
+   * @since 1.0.0
   */
  public function set_admin_color_based_on_env( $color_scheme ) {


Not sure we have this in the wiki, but it should be added, explaining what it does and why.

We don't have a lot of stuff in wiki, I will update this at some point

Or maybe @IvanGrginovInf can, since he has the gift of really writing things out in detail 😄😛

seeWhatIDidThere xD

#Downgrading. @dingo-d I'd love to update the wiki a bit tho I haven't used half of these things. Maybe once I start actually contributing. ;)

It's never too late to contribute :D

dingo-d · 2018-09-02T15:16:13Z

wp-content/themes/inf_theme/admin/class-media.php

   */
  public function add_custom_image_sizes() {
-    add_image_size( 'full_width', 9999, 9999, false );
+    add_image_size( 'full-width', 1440, 9999, false );


Wouldn't this limit the width to 1440px on full-width images? What if someone wants to put 2000px image in the background or something like that?

Yes, it will but 9999, 9999 was put there I don't know how.
Let's say 1440 is optimal width for full width. A developer should change this if needed

I agree that we should encourage optimization, but limiting this could cause issues opened with: why is my image not full size? :D Setting it to 9999 basically means, it will be whatever you put it (unless you put 30k px image xD)

if this is an issue then I will remove it because this actually does nothing. if you upload the image it will resize it to its max size which is its uploaded size. It is the same as full.

the_post_thumbnail( 'full' ); // Full resolution (original size uploaded)

dingo-d · 2018-09-02T15:18:19Z

wp-content/themes/inf_theme/admin/class-media.php

@@ -92,7 +68,7 @@ public function enable_svg_library_preview( $response, $attachment, $meta ) {
          $svg_content = file_get_contents( $path );


We should have a check

if ( ini_get( 'allow_url_fopen' ) ) {...}

Before using file_get_contents(), just as a precaution. And try to find alternative (wp_remote_get or cURL).

dingo-d · 2018-09-02T15:25:59Z

wp-content/themes/inf_theme/theme/class-theme.php

   */
  public function enqueue_scripts() {
    // jQuery.
    wp_deregister_script( 'jquery-migrate' );
    wp_deregister_script( 'jquery' );
-    wp_register_script( 'jquery', get_template_directory_uri() . '/skin/public/scripts/vendors/jquery.min.js', array(), '3.3.1' );
+    wp_register_script( 'jquery', get_template_directory_uri() . '/skin/public/scripts/vendors/jquery.min.js', array(), '3.3.1' ); // phpcs:ignore WordPress.WP.EnqueuedResourceParameters.NotInFooter


Likewise about not being in the footer.

in all WP themes, Jquery is never loaded in the footer so we shouldn't neither.

Just because they do it, doesn't mean it7s the right way to do it 😄 They are still using bundled jQuery (1.2) xD

I think we should put our scripts in footer, and make them dependent on the bundled vendor jquery. This way we are not blocking down the HTML parsing on the page (which will happen if the script is in the header).

If we put them in the header, we should investigate in adding async and defer attributes on our scripts.

Reference

I agree with you 100%, but we can't guarantee that plugin developers will hook their scripts to the footer or dependent on jQuery, and if the script is loaded in header and jQuery is in the footer it won't work.

let's leave this to the developers?, we can add an not in the readme?

How about adding it as an option? Radio button with "Load in header, load in header async, load in footer?"

Not a bad idea, but I think that it would be better to leave that to the users, and just present the minimal working version of the developers to use and enhance :)

…refactoring # Conflicts: # package-lock.json # package.json # wp-content/themes/inf_theme/functions.php

iruzevic · 2018-09-19T22:35:42Z

fixing merge conflicts with develop branch
removing file_get_content
creating new helper class Object_Helper that is going to be used as a Trait

@dingo-d please check.

and can we merge this into develop so it is easier to maintain with smaller changes before main release

…e call

dingo-d · 2018-09-20T07:20:36Z

wp-content/themes/inf_theme/admin/class-media.php


-          if ( ! $this->general_helper->is_valid_xml( $svg_content ) ) {
+          if ( ! static::is_valid_xml( $svg_content ) ) {


Replace static:: with $this-> since it's not a static method.

updating packages

4596b87

iruzevic requested a review from dingo-d August 1, 2018 12:55

global refactoring of css, js, php

be119de

iruzevic closed this Aug 2, 2018

iruzevic reopened this Aug 2, 2018

iruzevic added 10 commits August 3, 2018 00:35

Merge branch 'development' into feature/refactoring

b533ece

updating gitignore

8977446

Merge branch 'feature/refactoring' of github.com:infinum/wp-boilerpla…

1ee7355

…te into feature/refactoring

css quick fix

30181ae

adding quick fix

5886688

returning featured image :D

bd3d9d7

removing postcss-next

560acea

updating readme script

f2574fa

styling for rename script

b3b8a60

updating readme file

aac92b6

dingo-d reviewed Aug 8, 2018

View reviewed changes

packages updated, fixed creation of source maps, minor bug in class-m…

bee0ee3

…edia, and replacing normalize.css with sass version for lighter output, and fixed all terminal warnings

updating composer and fixing all linting errors and warnings

5824ee2

dingo-d reviewed Sep 2, 2018

View reviewed changes

updating readme

f143933

dingo-d reviewed Sep 2, 2018

View reviewed changes

iruzevic added 2 commits September 2, 2018 17:00

removing bugsnag

a671cf7

updating copy

25f5f36

dingo-d reviewed Sep 2, 2018

View reviewed changes

loading scrips in footer

2f85f80

dingo-d reviewed Sep 2, 2018

View reviewed changes

infinum deleted a comment from iruzevic Sep 2, 2018

iruzevic added 3 commits September 2, 2018 17:48

removing media size

34a8bbf

implementing object helper and removing file_get_content

f3a88a3

Merge commit '7bc91268d18f2cf36eea999a8bf74ee3c9f62ba3' into feature/…

81cd6c1

…refactoring # Conflicts: # package-lock.json # package.json # wp-content/themes/inf_theme/functions.php

iruzevic and others added 3 commits September 20, 2018 09:21

fix

514997b

Added ignore rule in the standards

e346184

Added ignore rule about script in footer, mninor refactor of multilin…

ebe6a2d

…e call

dingo-d approved these changes Sep 20, 2018

View reviewed changes

iruzevic merged commit 9af0f78 into development Sep 20, 2018

iruzevic deleted the feature/refactoring branch September 20, 2018 07:53


		* Add this aliases to you bash config:
		3. Linting PHP - If you have composer installed add this aliases to you bash config and reload terminal:


		If you are using VVV clone it in the `public_html` folder
		```bash

		@@ -92,7 +68,7 @@ public function enable_svg_library_preview( $response, $attachment, $meta ) {
		$svg_content = file_get_contents( $path );


		if ( ! $this->general_helper->is_valid_xml( $svg_content ) ) {
		if ( ! static::is_valid_xml( $svg_content ) ) {

updating packages #99

updating packages #99

Conversation

iruzevic commented Aug 1, 2018 • edited Loading

iruzevic commented Aug 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iruzevic commented Sep 1, 2018

iruzevic commented Sep 1, 2018

Choose a reason for hiding this comment

iruzevic Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iruzevic Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iruzevic Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iruzevic commented Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

iruzevic commented Aug 1, 2018 •

edited

Loading

iruzevic Sep 2, 2018 •

edited

Loading

iruzevic Sep 2, 2018 •

edited

Loading

iruzevic Sep 2, 2018 •

edited

Loading

iruzevic commented Sep 19, 2018 •

edited

Loading