Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Create renderFrontend utils function #954

Merged
merged 2 commits into from
Sep 9, 2019
Merged

Create renderFrontend utils function #954

merged 2 commits into from
Sep 9, 2019

Conversation

Aljullu
Copy link
Contributor

@Aljullu Aljullu commented Sep 9, 2019

This PR creates a renderFrontend utils function so we avoid duplicating code in frontend.js files for each new component.

How to test the changes in this Pull Request:

  1. Create a post and add a Product Categories List, All Reviews, Reviews by Product and Reviews by Category blocks.
  2. Preview the post and verify blocks are displayed correctly and attributes are honored.

Changelog

Create renderFrontend utils function to avoid duplicating code for each block.

@Aljullu Aljullu added status: needs review type: refactor The issue/PR is related to refactoring. labels Sep 9, 2019
@Aljullu Aljullu added this to the 2.5 milestone Sep 9, 2019
@Aljullu Aljullu requested a review from a team September 9, 2019 11:10
@Aljullu Aljullu self-assigned this Sep 9, 2019
Copy link
Contributor

@nerrad nerrad left a comment

Choose a reason for hiding this comment

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

Nice encapsulation work here. I have a couple comments though.

const containers = document.querySelectorAll(
'.wp-block-woocommerce-product-categories'
);
const selector = '.wp-block-woocommerce-product-categories';
Copy link
Contributor

Choose a reason for hiding this comment

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

Declaring this variable here is unnecessary, can just reference the selector directly in the renderFrontend call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 454c5d7.


if ( containers.length ) {
// Use Array.forEach for IE11 compatibility
Array.prototype.forEach.call( containers, ( el ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, why not just containers.forEach which is functionally equivalent because containers is an array? I see the comment about "for IE11" compatibility but won't babel transpile handle that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

containers is a NodeList and IE11 doesn't support the forEach method:
https://developer.mozilla.org/en-US/docs/Web/API/NodeList/forEach#Browser_Compatibility

I also thought Babel and the polyfills would take care of it, but it looks like they don't (just tested with IE11): #, #.

Copy link
Contributor

Choose a reason for hiding this comment

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

containers is a NodeList

Ahh right! Thanks for verifying with polyfills too 👍

@Aljullu
Copy link
Contributor Author

Aljullu commented Sep 9, 2019

Thanks for the review @nerrad! This is ready for another look.

@Aljullu Aljullu merged commit 11ba8c3 into master Sep 9, 2019
@Aljullu Aljullu deleted the add/render-frontend branch September 9, 2019 13:25
@Aljullu Aljullu added the skip-changelog PRs that you don't want to appear in the changelog. label Nov 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip-changelog PRs that you don't want to appear in the changelog. type: refactor The issue/PR is related to refactoring.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants