load-more component #26

goranalkovic-infinum · 2021-01-21T09:11:43Z

No description provided.

iruzevic

src/Blocks/components/load-more/load-more-style.scss empty files should be removed

src/Blocks/components/load-more/assets/index.js

iruzevic · 2021-01-21T09:48:05Z

src/Blocks/components/load-more/assets/index.js

+	for (let element of elements) {
+		const url = element.dataset.url;
+		const containerClass = element.dataset.container;
+		const button = element.querySelector('button');


all js selectors must have .js- prefix

Is .js-load-more load-more__button ok for a selector?

you should only use .js-load-more

In this case I need to target the button so I can add an event listener to it, though.

.js-load-more is in the top component div.

iruzevic · 2021-01-21T09:49:37Z

src/Blocks/components/load-more/assets/index.js

@@ -0,0 +1,33 @@
+import domReady from '@wordpress/dom-ready';


all js files must be written using dynamic Import for optimisations purposes.
Please refer to the carousel assets or any other assets in the 8shift-frontend-libs or solplanet project

That's exactly how it is in the 8shift-frontend libs Carousel block 😁

Is this the correct way to do it?

async function load() { let domReady = await import('@wordpress/dom-ready'); domReady.default(() => { // ... }); } load();

Why did you use async await here?

this is allready dynamic import
https://github.com/infinum/eightshift-frontend-libs/blob/c84e4dd0af2ff05cd2cd7bcc62e5e0097c39ed55/blocks/init/src/Blocks/custom/carousel/assets/index.js#L10

read about this:
https://medium.com/front-end-weekly/webpack-and-dynamic-imports-doing-it-right-72549ff49234

https://medium.com/front-end-weekly/webpack-and-dynamic-imports-doing-it-right-72549ff49234

I use async/await because

let bar = await foo(); // use bar

should be the same as

foo().then((bar) => { // use bar });

I like async/await more because it works the same, but removes the unnecessary indentation and brackets.
(same with x.forEach((y) => {}) and for (let y of x) {} 😄 )

But if .then() is how it's done everywhere, I'll change it to be consistent with everything.

BTW in the Carousel example, domReady isn't being dynamically imported, right?

We are using webpack dynamic import because then webpack handles when the file is actually loaded in the DOM witch reduces the JS file sizes.

Please use carousel as reference.

No domReady is not dynamically imported

If I understood you correctly, I should move most of the logic into a separate file and then in index.js dynamically import it?

Unlike Carousel, I don't need to import anything external, all of the loading logic is pure async JS.

Should domReady be dynamically imported or is it ok if it stays like it is in Carousel?

Please just check this other examples and make it like this:

https://github.com/infinum/wordpress-solplanet/tree/master/wp-content/themes/solplanet/src/Blocks/custom/carousel/assets

https://github.com/infinum/wordpress-solplanet/blob/master/wp-content/themes/solplanet/src/Blocks/custom/cards-grid/assets/index.js

https://github.com/infinum/wordpress-solplanet/tree/master/wp-content/themes/solplanet/src/Blocks/custom/featured-categories-posts/assets

src/Blocks/components/load-more/assets/index.js

iruzevic · 2021-01-21T09:50:29Z

src/Blocks/components/load-more/components/load-more-editor.js

+		<Fragment>
+			{loadMoreUse &&
+				<div className={buttonWrapClass}>
+					<i>{loadMoreButtonLabel}</i>


thy the tag here?

Should've been p

You dont need any tag here

This has since been replaced with ~~a RichText to make it changeable live in editor, like the button itself.~~ the button editor.

iruzevic · 2021-01-21T09:50:54Z

src/Blocks/components/load-more/components/load-more-editor.js

+	return (
+		<Fragment>
+			{loadMoreUse &&
+				<div className={buttonWrapClass}>


if this is a button use button not div. We must follow the correct semantics

iruzevic · 2021-01-21T09:52:14Z

src/Blocks/components/load-more/docs/story.js

@@ -0,0 +1,90 @@
+import React from 'react';


please fix this story as I see in the code you dont have nothing similar to this

iruzevic · 2021-01-22T11:42:38Z

src/Blocks/components/load-more/assets/index.js

@@ -0,0 +1,53 @@
+
+async function load() { 


you have overcomplicated this please read the provided docs in the comment and see how Js is done on other blocks and projects

iruzevic · 2021-01-22T11:43:53Z

src/Blocks/components/load-more/components/load-more-editor.js

+		<Fragment>
+			{loadMoreUse &&
+				<Fragment>
+					<div style={{


never ever ever use inline css

iruzevic · 2021-01-22T11:44:12Z

src/Blocks/components/load-more/components/load-more-editor.js

+							{componentTitle}
+						</p>
+						<small><i>{componentDescription}</i></small>
+						<br />


also br for spacing is bit no no

iruzevic · 2021-01-22T11:44:44Z

src/Blocks/components/load-more/components/load-more-editor.js

+						<br />
+						<br />
+
+						<div className="btn-wrap btn-wrap__align--center">


why not use button component here if you are using button?*

Guess I ~~could~~ should, I did it like this because I didn't think of any user customization of the button :D

iruzevic · 2021-01-22T11:45:13Z

src/Blocks/components/load-more/components/load-more-options.js

+
+
+			{loadMoreUse &&
+				<Fragment>


indention wrong

iruzevic · 2021-01-22T11:47:22Z

src/Blocks/components/load-more/components/load-more-options.js

+						/>
+						<TextControl
+							label="Starting item index URL parameter"
+							value={loadMoreStartItemParameterName ?? ''}


iruzevic · 2021-01-22T11:47:25Z

src/Blocks/components/load-more/components/load-more-options.js

+						<TextControl
+							label="Starting item index URL parameter"
+							value={loadMoreStartItemParameterName ?? ''}
+							onChange={(value) => setAttributes({ [`loadMoreStartItemParameterName`]: value })}


iruzevic · 2021-01-22T11:48:06Z

src/Blocks/components/load-more/load-more.php

+use UnicornsVendor\EightshiftLibs\Helpers\Components;
+
+$manifest = Components::getManifest(__DIR__);
+


where is use check here?

iruzevic · 2021-01-22T11:48:20Z

src/Blocks/components/load-more/load-more.php

+?>
+
+<div class="<?php echo \esc_attr($loadMoreClass); ?>" data-url="<?php echo \esc_url_raw($loadMoreUrl); ?>" data-paginated="<?php echo \esc_attr($loadMoreUsePagination); ?>" data-per-page="<?php echo \esc_attr($loadMoreItemsPerPage); ?>" data-per-page-param="<?php echo \esc_attr($loadMoreItemsPerPageParameterName); ?>" data-start-item="<?php echo \esc_attr($loadMoreStartItem); ?>" data-start-item-param="<?php echo \esc_attr($loadMoreStartItemParameterName); ?>">
+	<div class="<?php echo \esc_attr($componentJsClass) ?>--container"></div>


comment in the previous PR

iruzevic · 2021-01-22T11:48:48Z

src/Blocks/components/load-more/load-more.php

+	<div class="<?php echo \esc_attr($componentJsClass) ?>--container"></div>
+
+	<?php
+	echo \wp_kses_post(Components::render('button', array_merge(


you are using button component here and in JS you didn't

iruzevic · 2021-01-26T08:58:49Z

src/Blocks/components/load-more/load-more.php

@@ -29,6 +29,7 @@
 $loadMoreItemsPerPageParameterName = Components::checkAttr('loadMoreItemsPerPageParameterName', $attributes, $manifest, $componentName);
 $loadMoreStartItem = Components::checkAttr('loadMoreStartItem', $attributes, $manifest, $componentName);
 $loadMoreStartItemParameterName = Components::checkAttr('loadMoreStartItemParameterName', $attributes, $manifest, $componentName);
+$loadMoreInitialLoad = $attributes['loadMoreInitialLoad'] ?? 'false';


use checkAttr functionality here na put this in the manifest

iruzevic · 2021-01-26T08:59:11Z

src/Blocks/components/load-more/load-more.php

@@ -38,7 +39,7 @@

 ?>

-<div class="<?php echo \esc_attr($loadMoreClass); ?>" data-url="<?php echo \esc_url_raw($loadMoreUrl); ?>" data-paginated="<?php echo \esc_attr($loadMoreUsePagination); ?>" data-per-page="<?php echo \esc_attr($loadMoreItemsPerPage); ?>" data-per-page-param="<?php echo \esc_attr($loadMoreItemsPerPageParameterName); ?>" data-start-item="<?php echo \esc_attr($loadMoreStartItem); ?>" data-start-item-param="<?php echo \esc_attr($loadMoreStartItemParameterName); ?>">
+<div class="<?php echo \esc_attr($loadMoreClass); ?>" data-initial-load="<?php echo \esc_attr($loadMoreInitialLoad); ?>" data-url="<?php echo \esc_url_raw($loadMoreUrl); ?>" data-paginated="<?php echo \esc_attr($loadMoreUsePagination); ?>" data-per-page="<?php echo \esc_attr($loadMoreItemsPerPage); ?>" data-per-page-param="<?php echo \esc_attr($loadMoreItemsPerPageParameterName); ?>" data-start-item="<?php echo \esc_attr($loadMoreStartItem); ?>" data-start-item-param="<?php echo \esc_attr($loadMoreStartItemParameterName); ?>">


brake this in multiple lines so it is easy to read

iruzevic · 2021-01-27T08:30:43Z

src/Blocks/components/load-more/components/load-more-toolbar.js

+    } = attributes;
+
+    return (
+        <Fragment>


if you have only one top level item you don't need fragment

iruzevic · 2021-01-27T08:31:18Z

src/Blocks/components/load-more/load-more.php

+		]
+	));
+
+	$buttonHtml = str_replace("{$componentClass}__button", "{$componentJsClass}-button", $buttonHtml);


again never do this

Should I fix this with a render_block_data filter or?

The only other option I can think of is to modify the button to allow passing extra classes.

iruzevic · 2021-01-27T08:32:18Z

src/Blocks/custom/load-more/components/load-more-editor.js

@@ -0,0 +1,11 @@
+import React from 'react';


why do you need load more block?

you are never going to use it as a block. please remove it

iruzevic · 2021-01-27T08:32:54Z

src/Blocks/components/load-more/components/load-more-editor.js

+			{loadMoreUse &&
+				<Fragment>
+					<div className={`${componentClass}__editor-wrapper`}>
+						<p className={`${componentClass}__editor-wrapper-title`}>


what is this title and description? If you have decription for the component put it in the options sidebar

I wanted to make the block a bit nicer in the editor, but I can remove it.

Goran Alković added 3 commits January 20, 2021 12:48

add load-more component

a0cc58c

center the button in the Load more component

6dd3325

make Load more have pagination

73bbaf3

goranalkovic-infinum requested a review from iruzevic January 21, 2021 09:11

iruzevic requested changes Jan 21, 2021

View reviewed changes

Goran Alković added 13 commits January 21, 2021 15:00

remove empty scss file

e274687

remove extra spaces

9d9966b

remove unnecessary toolbar editor

ff65279

add missing entries to manifest

a27cc9c

clean up editor view of the component

b62d15a

clean up editor file

f0739e2

add new attributes to manifest

dbdca16

clean up index.js and use dynamic import

d6f06ff

add new options for new attributes

bb95602

update story to match component

efdcc84

add new attributes to php side

183ab7d

add missing selectorClass

2cb079c

fix 'load more' button selector

09efca0

goranalkovic-infinum requested a review from iruzevic January 22, 2021 11:08

iruzevic requested changes Jan 22, 2021

View reviewed changes

Goran Alković added 10 commits January 25, 2021 12:53

make button use non-BEM class I need in JS

5d8d146

remove the dynamic domReady import

19d226a

add editor styles

a3a953d

add button component to manifest

e8796f6

add toolbar

9048a80

remove inline styles and use button editor

6921090

clean up php

40fdb81

add button options

a73ce1e

remove fallbacks

e98ed6c

make strings translatable

e1565cf

Goran Alković added 4 commits January 25, 2021 13:48

use non-hardcoded component name

e2e14d1

make container class non-BEM

9bbde9a

add use check to php

83d3801

fix wrong variable name

f76ed75

goranalkovic-infinum requested a review from iruzevic January 25, 2021 12:59

Goran Alković added 7 commits January 25, 2021 14:54

seperated logic into second file, dynamic imported

b31e24e

use forEach instead of for...of and async/await

e64622e

change textinput for URLInput for url

9eb1098

fix incorrect localization args

3500019

add initialLoad attribute

830c9fd

add option to hide button's use toggle

075ee12

hide unncessary button options controls

a7adbe8

iruzevic requested changes Jan 26, 2021

View reviewed changes

Goran Alković added 2 commits January 26, 2021 11:30

move data-attrs into seperate lines

58be69e

use loadMoreInitialLoad via checkAttr

a2172c4

goranalkovic-infinum requested a review from iruzevic January 26, 2021 10:39

fix data attr not being populated correctly

c80d2cb

iruzevic requested changes Jan 27, 2021

View reviewed changes

		@@ -0,0 +1,33 @@
		import domReady from '@wordpress/dom-ready';

		use UnicornsVendor\EightshiftLibs\Helpers\Components;

		$manifest = Components::getManifest(__DIR__);

load-more component #26

Are you sure you want to change the base?

load-more component #26

Conversation

goranalkovic-infinum commented Jan 21, 2021

iruzevic left a 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

goranalkovic-infinum Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goranalkovic-infinum Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

iruzevic Jan 25, 2021 • 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

goranalkovic-infinum Jan 25, 2021 • 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

Choose a reason for hiding this comment

goranalkovic-infinum Jan 22, 2021 •

edited

Loading

goranalkovic-infinum Jan 25, 2021 •

edited

Loading

iruzevic Jan 25, 2021 •

edited

Loading

goranalkovic-infinum Jan 25, 2021 •

edited

Loading