feat: add ability to configure lazy loading #147

rostik404 · 2018-10-23T13:25:41Z

No description provided.

sipayRT · 2018-10-23T13:58:07Z

lib/static/components/controls/common-controls.js

@@ -58,6 +58,11 @@ class ControlButtons extends Component {
                    isActive={view.scaleImages}
                    handler={actions.toggleScaleImages}
                />
+                <ControlButton
+                    label="Lazy load"


я бы тут добавил именно 'Lazy image load'

А я бы назвал Lazy image loading

sipayRT · 2018-10-23T14:04:53Z

lib/static/components/state/screenshot.js

-                />
-            </LazyLoad>
-        );
+        const elem = <img


давай в одну строку - тут же кода понты

sipayRT · 2018-10-23T14:06:53Z

lib/static/modules/default-state.js

@@ -13,7 +13,8 @@ export default {
    config: {
        defaultView: 'all',
        scaleImages: false,
-        baseHost: ''
+        baseHost: '',
+        lazyLoadOffset: 800


какая-то у нас тут херь происходит - получается, что у нас дефолты указываются в двух местах (в конфиге плагина и здесь)

Согласен, полная херня. Должен быть один источник правды

sipayRT · 2018-10-23T14:39:48Z

кстати, а тесты то где? :)
на реактовский компонент напиши типа https://github.com/gemini-testing/html-reporter/blob/master/test/lib/static/components/section/state.js#L79

DudaGod

Мне не нравится, что в итоге у нас есть 2 опции lazyLoad и lazyLoadOffset. Почему не достаточно сделать одну опцию? Плюс из конфига никак нельзя вообще отключить lazyLoad. Он всегда выставлен в true по умолчанию. Можно было бы при клике на кнопку Lazy load выставлять lazyLoadOffset в ноль.

DudaGod · 2018-10-23T14:53:11Z

README.md

@@ -23,6 +23,7 @@ directory.
  * `failed` - show only failed tests.
 * **baseHost** (optional) - `String` - it changes original host for view in the browser; by default original host does not change
 * **scaleImages** (optional) – `Boolean` – fit images into page width; `false` by default
+* **lazyLoadOffset** (optional) - `Number` - allows you to specify how far above and below the viewport you want to begin loading images


значение по умолчанию не указал

Может сделаем вложенность небольшую?

lazyLoad: { enabled: true, offset: 100500 }

DudaGod · 2018-10-23T15:00:12Z

lib/config.js

@@ -45,6 +46,10 @@ const getParser = () => {
            parseEnv: JSON.parse,
            parseCli: JSON.parse,
            validate: assertBoolean('scaleImages')
+        }),
+        lazyLoadOffset: option({
+            defaultValue: 800,


кажется пора заводить отдельный файл с значениями по умолчанию

DudaGod · 2018-10-23T15:01:48Z

lib/static/components/controls/common-controls.js

@@ -58,6 +58,11 @@ class ControlButtons extends Component {
                    isActive={view.scaleImages}
                    handler={actions.toggleScaleImages}
                />
+                <ControlButton
+                    label="Lazy load"


А я бы назвал Lazy image loading

DudaGod · 2018-10-23T15:09:21Z

lib/static/modules/default-state.js

@@ -13,7 +13,8 @@ export default {
    config: {
        defaultView: 'all',
        scaleImages: false,
-        baseHost: ''
+        baseHost: '',
+        lazyLoadOffset: 800


Согласен, полная херня. Должен быть один источник правды

DudaGod · 2018-10-23T15:10:22Z

lib/static/components/state/index.js

-        scaleImages: PropTypes.bool
+        scaleImages: PropTypes.bool,
+        lazyLoad: PropTypes.bool,
+        lazyLoadOffset: PropTypes.number


не понял этот дифф. Ты же эти поля из стейта тут не достаешь и никак не используешь

DudaGod · 2018-10-23T15:21:47Z

lib/static/components/state/screenshot.js

+            );
+        }
+
+        return elem;


давай через тернарник напишем, красивее же будет

DudaGod · 2018-10-23T15:23:08Z

lib/static/components/state/screenshot.js

    }
 }

+export default connect(({view: {lazyLoad, lazyLoadOffset}}) => ({lazyLoad, lazyLoadOffset}))(Screenshot);


у тебя lazyLoadOffset должен браться из config, а не из view

DudaGod · 2018-10-23T15:25:04Z

lib/static/modules/action-names.js

@@ -20,5 +20,6 @@ export default {
    VIEW_TOGGLE_SKIPPED: 'VIEW_TOGGLE_SKIPPED',
    VIEW_TOGGLE_ONLY_DIFF: 'VIEW_TOGGLE_ONLY_DIFF',
    VIEW_UPDATE_BASE_HOST: 'VIEW_UPDATE_BASE_HOST',
-    VIEW_TOGGLE_SCALE_IMAGES: 'VIEW_TOGGLE_SCALE_IMAGES'
+    VIEW_TOGGLE_SCALE_IMAGES: 'VIEW_TOGGLE_SCALE_IMAGES',
+    VIEW_TOGGLE_LAZY_LOAD: 'VIEW_TOGGLE_LAZY_LOAD'


я бы тут images тоже добавил бы

DudaGod · 2018-10-23T15:29:20Z

lib/static/modules/reducer.js

@@ -22,6 +22,7 @@ function getInitialState(compiledData) {
        view: {
            viewMode: config.defaultView,
            scaleImages: config.scaleImages,
+            lazyLoadOffset: config.lazyLoadOffset,


а для чего так делать? В компоненте screenshot бери это значение просто из config. К view же это не относится. К нему относится только lazyLoad

После того, как реализовал через одну опцию, это стало необходимо выгружать во view, т.к. это уже не просто константа.

DudaGod · 2018-10-23T15:39:51Z

lib/static/modules/reducer.js

            const formattedSuites = formatSuitesData(suites);

-            return merge({}, state, {gui, autoRun, skips, view: {scaleImages}}, formattedSuites);
+            return merge({}, state, {gui, autoRun, skips, view: {scaleImages, lazyLoadOffset}}, formattedSuites);


а lazyLoad куда потерял? Ты же его тоже должен прокинуть.
Плюс очень путает, что ты взял значение из конфига и прокинул его во view. Хотя тебе это делать вообще не нужно

уже не актуально

sipayRT · 2018-10-24T12:23:58Z

идея с одним параметром норм

sipayRT

в остальном ок

sipayRT · 2018-10-25T08:29:23Z

test/lib/config.js

@@ -136,4 +136,30 @@ describe('config', () => {
            assert.isTrue(parseConfig({}).scaleImages);
        });
    });
+
+    describe('"lazyLoadOffset" option', () => {
+        it('should has default balue', () => {


⚠️ balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue, balue ⚠️

sipayRT · 2018-10-25T08:30:24Z

test/lib/config.js

+
+    describe('"lazyLoadOffset" option', () => {
+        it('should has default balue', () => {
+            assert.equal(parseConfig({}).lazyLoadOffset, 800);


вместо 800 укажи значение из дефолтов

sipayRT · 2018-10-25T08:31:11Z

test/lib/config.js

@@ -136,4 +136,30 @@ describe('config', () => {
            assert.isTrue(parseConfig({}).scaleImages);
        });
    });
+
+    describe('"lazyLoadOffset" option', () => {


добавь тест на то, что можно предавать только числа

sipayRT · 2018-10-25T08:33:05Z

test/lib/static/components/section/state.js

+            assert.lengthOf(lazyLoadContainer, 0);
+        });
+
+        it('should not load images lazy of lazy load offset is not specified', () => {


этот тест у тебя сейчас ничем не отличается от предыдущего

sipayRT approved these changes Oct 23, 2018

View reviewed changes

DudaGod requested changes Oct 23, 2018

View reviewed changes

rostik404 force-pushed the fix-lazy-load branch from 9a76434 to c965803 Compare October 25, 2018 08:14

sipayRT approved these changes Oct 25, 2018

View reviewed changes

feat: add ability to configure lazy loading

dbd0a0d

rostik404 force-pushed the fix-lazy-load branch from c965803 to dbd0a0d Compare October 25, 2018 09:16

rostik404 merged commit ada7628 into master Oct 25, 2018

rostik404 deleted the fix-lazy-load branch October 25, 2018 09:27

dependencies bot mentioned this pull request Oct 25, 2018

Update 3 dependencies from npm JetBrains/ring-ui#466

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add ability to configure lazy loading #147

feat: add ability to configure lazy loading #147

rostik404 commented Oct 23, 2018

sipayRT Oct 23, 2018

DudaGod Oct 23, 2018

sipayRT Oct 23, 2018

sipayRT Oct 23, 2018

DudaGod Oct 23, 2018

sipayRT commented Oct 23, 2018

DudaGod left a comment

DudaGod Oct 23, 2018

DudaGod Oct 23, 2018

DudaGod Oct 23, 2018

DudaGod Oct 23, 2018

DudaGod Oct 23, 2018

DudaGod Oct 23, 2018

DudaGod Oct 23, 2018

DudaGod Oct 23, 2018

DudaGod Oct 23, 2018

DudaGod Oct 23, 2018

rostik404 Oct 25, 2018

DudaGod Oct 23, 2018

rostik404 Oct 25, 2018

sipayRT commented Oct 24, 2018

sipayRT left a comment

sipayRT Oct 25, 2018

sipayRT Oct 25, 2018

sipayRT Oct 25, 2018

sipayRT Oct 25, 2018

feat: add ability to configure lazy loading #147

feat: add ability to configure lazy loading #147

Conversation

rostik404 commented Oct 23, 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

sipayRT commented Oct 23, 2018

DudaGod 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

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

sipayRT commented Oct 24, 2018

sipayRT 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