Added initialization logic when changing container attributes #162

devxom · 2019-12-25T23:56:54Z

Added logic to manual update the Likely instance when changing the data attributes of the container on which Likely is initialized

Changes:

Added the updateOptionFromDataSet method to update the Likely instance configuration after from the data attributes after initialization
Added an html page for the ability to manually and automatically test data updates when changing data attributes
Added automatic tests to verify that after changing the data-url attribute on the widget container when opening the model window for sharing, the updated url from the data-url attribute on the container is used
Added an example to README.md to use the logic for updating the configuration from the date of attributes after initializing the Likely instance

Fixed: #151

Previous version of description

Added logic to initialize the library when changing the data attributes of the container on which Likely is initialized

Changes:

When initializing a shared sharing widget, the creation of MutationObserver was added to change the attributes of the widget container
Added an html page for the ability to manually and automatically test data updates when changing data attributes
Added automatic tests to verify that after changing the data-url attribute on the widget container when opening the model window for sharing, the updated url from the data-url attribute on the container is used

Important: to determine that the data-url attribute on the container has changed, it uses the MutationObserver API which works on a slightly narrower number of browsers. In browsers without support for this API, just this logic will not work.

Fixed: #151

vitkarpov · 2019-12-26T14:22:20Z

test/index.js

+            { name: 'Telegram', likelyName: 'telegram', urlRegex: `${encodeURIComponent(targetUrl)}` },
+            { name: 'Twitter', likelyName: 'twitter', urlRegex: `${encodeURIComponent(targetUrl)}` },
+            { name: 'VK', likelyName: 'vkontakte', urlRegex: `${encodeURIComponent(targetUrl)}` },
+            // Pinterest and Linkedin do not contain sharing URLs if not authorized and the test will fail


Just out of curiosity, wouldn't this happen for these buttons in production?

This is a problem only for this particular auto test. We cannot determine that the data has been verified. It's not a problem.

vitkarpov · 2019-12-26T14:26:58Z

In browsers without support for this API, just this logic will not work.

Could users initialize the widget once again manually?

NikolayRys · 2019-12-26T20:47:09Z

Hi, @devxom, thank you for the PR, I appreciate your effort.

However, after some consideration, I've got a feeling that the proposed solution might be unnecessarily complicated. Namely, I think it's sufficient to provide the client with something like likely.reinitialize or likely.updateOptions to use when the attributes are changed.

If the user needs to detect it automatically, they can add a Mutation Observer approach on their own. In the scope of your pull request, I suggest to keep the content of "updateOptionFromDataSet" and remove the observer.

What do you think?

devxom · 2019-12-29T18:17:15Z

@NikolayRys, i agree. Adding DOM attribute tracking logic is unnecessarily complicated in the library itself.

Within a few days I will update the pull request. Where from the edits will be:

logic updateOptionFromDataSet
tests
an example in README.md that will show how, for example, you can use MutationsObserver from outside the library

NikolayRys · 2019-12-29T21:45:28Z

Sounds awesome, thank you.

devxom · 2020-01-06T16:14:53Z

Updated patch and description for pull request. Review please.

vitkarpov

@devxom @NikolayRys guys, what do you think about to not introduce a new method but use existing init? It's like there's only one way to (re)initialize the widget.

vitkarpov · 2020-01-06T18:29:05Z

readme.md

+
+3. Some logic for update data attributes
+
+2. Logic for update configuration on instance *Likely*


It's weird but the list is rendered as 1, 3, 4 (check out the preview on GitHub) 🤔
I don't get the second item, "some logic to update attributes", could you elaborate, pls?

I will rewrite the example so that it displays normally.

Please complete the description.

vitkarpov · 2020-01-06T18:30:21Z

readme.md

+2. Logic for update configuration on instance *Likely*
+
+```javascript
+// find in dom likely


Probably it's better to call it "the DOM"? (like developers.mozilla does)

Please use "Find Likely in the DOM"

vitkarpov · 2020-01-06T18:31:16Z

source/widget.js

@@ -37,6 +37,13 @@ class Likely {
        }
    }

+    updateOptionFromDataSet() {


Shouldn't it be plural, I mean updateOptionsFromDataSet?

Please use plural.

devxom · 2020-01-07T21:04:54Z

@devxom @NikolayRys guys, what do you think about to not introduce a new method but use existing init? It's like there's only one way to (re)initialize the widget.

It sounds logical, I tested it works well. I updated the edits leaving only an example and tests.

vitkarpov · 2020-01-09T13:19:48Z

@devxom sorry, I'm not sure I get it right. Are you going to change the code in a way it uses init only or leave it as-is? (the latter is fine, just asking)

devxom · 2020-01-09T14:10:48Z

@vitkarpov I liked the idea of such a simplification, I will update the edits in the near future using init

vitkarpov · 2020-01-10T07:22:24Z

Gotcha, thanks! 👍

NikolayRys

@devxom Sorry for the delay, I was caught up with other projects.
It's pretty good now, please do the quested fixes - they are all pretty much cosmetical, while the feature itself is mergeable.

NikolayRys · 2020-02-05T11:41:24Z

readme.md

+2. Logic for update configuration on instance *Likely*
+
+```javascript
+// find in dom likely


Please use "Find Likely in the DOM"

NikolayRys · 2020-02-05T11:44:42Z

readme.md

+
+3. Some logic for update data attributes
+
+2. Logic for update configuration on instance *Likely*


Please complete the description.

NikolayRys · 2020-02-05T11:45:24Z

readme.md

+// find in dom likely
+const likelyWidget = document.querySelector(".likely");
+
+// find instance of likely 


"Get the instance of Likely"

NikolayRys · 2020-02-05T11:46:05Z

readme.md

+// find instance of likely 
+const likelyInstance = likelyWidget.likely;
+
+// Call inner method for update options from data attributes


"...to update options from the data attributes"

NikolayRys · 2020-02-05T11:46:28Z

source/widget.js

@@ -37,6 +37,13 @@ class Likely {
        }
    }

+    updateOptionFromDataSet() {


Please use plural.

NikolayRys · 2020-02-11T16:39:58Z

test/utils/getLikelyPage.js

@@ -5,6 +5,7 @@ const LikelyPage = {
    NO_AUTOINIT: 'no-autoinit.html',
    NO_AUTOINIT_MULTIPLE: 'no-autoinit-multiple.html',
    ISSUE_67: 'issues/67.html',
+    ISSUE_151: 'issues/151.html',


Those names for the tests and pages aren't great. Let's please use some more descriptive as UPDATE_OPTIONS: 'update-options.html'.
I will eventually refactor that 67.html too.

NikolayRys · 2020-03-25T02:10:49Z

@devxom @vitkarpov Guys, I need your help.
I see that we have a test that covers changing attributes with re-initialization after that: https://github.com/NikolayRys/Likely/blob/master/test/index.js#L158

Here's where re-initialization currently happen:
https://github.com/NikolayRys/Likely/blob/master/source/index.js#L32

Could you help me figure out, does this pull request add any capabilities beyond that?

vitkarpov · 2020-03-25T04:25:23Z

@NikolayRys What do you mean? What capabilities exactly?

NikolayRys · 2020-03-25T05:01:47Z

I'm under impression that we already have a completely functional re-initialization logic in place, could you please confirm/deny this?
If it is indeed the case, then do we still need the separate method from this PR that updates attributes from the HTML tag?

vitkarpov · 2020-03-25T17:01:44Z

Yeah, I see what you mean. It seems that this PR doesn't do anything new 🤔I think we need to reproduce what #151 reports, probably it's already fine.

devxom · 2020-03-25T17:29:18Z

Really necessary logic to update configuration from date attributes. In this form, Pull Request does not make sense.

@NikolayRys @vitkarpov How do you look at simply describing the approach to do when using dynamic date attributes and adding a test that checks and shows how this happens?

vitkarpov · 2020-03-25T17:45:06Z

Sounds good to me! 👍 -- Cheers, Viktor

NikolayRys · 2020-03-25T18:18:59Z

We definitely lacked this information about that in the readme, so at least this part of work in this PR can be kept.

@devxom But about the test - doesn't this test: https://github.com/NikolayRys/Likely/blob/master/test/index.js#L158 already cover it?

devxom · 2020-03-27T08:26:38Z

@NikolayRys

@devxom But about the test - doesn't this test: https://github.com/NikolayRys/Likely/blob/master/test/index.js#L158 already cover it?

You are right, indeed this test covers this case

This weekend I will update the Pull Request. I will remove unnecessary edits and update changes with edits in README.md with an explanation of how to reinitialize Likely when using dynamic date attributes

NikolayRys · 2020-04-29T03:23:30Z

@devxom No update yet?

Signed-off-by: Ilya Reshetnikov <ilya.reshetnikov@devxom.tech>

devxom · 2020-05-01T00:25:12Z

@NikolayRys Reassembled edits based on comments. Please take another look at the code.

vitkarpov

There are some notes about typos & grammar in README. Have you looked at them? Probably we decided to disregard, I don't remember.

NikolayRys · 2020-05-05T13:31:47Z

Hi, @devxom, thanks for taking care of this.

devxom mentioned this pull request Dec 26, 2019

Error dynamic data-url params #151

Closed

vitkarpov reviewed Dec 26, 2019

View reviewed changes

NikolayRys self-assigned this Dec 26, 2019

NikolayRys mentioned this pull request Jan 1, 2020

Version 3 epic #163

Closed

14 tasks

vitkarpov reviewed Jan 6, 2020

View reviewed changes

NikolayRys requested changes Feb 11, 2020

View reviewed changes

NikolayRys mentioned this pull request Apr 29, 2020

Version 2.5 epic #173

Closed

13 tasks

devxom force-pushed the feature/add-observable-data-options branch from 2e41c38 to 1d634be Compare April 30, 2020 23:46

Added a description for the Likely instance update logic

f3e7678

Signed-off-by: Ilya Reshetnikov <ilya.reshetnikov@devxom.tech>

devxom force-pushed the feature/add-observable-data-options branch from 1d634be to f3e7678 Compare May 1, 2020 00:22

devxom requested a review from NikolayRys May 1, 2020 00:25

devxom requested a review from vitkarpov May 3, 2020 01:32

vitkarpov reviewed May 3, 2020

View reviewed changes

NikolayRys merged commit 549f56d into NikolayRys:master May 5, 2020


		3. Some logic for update data attributes

		2. Logic for update configuration on instance Likely

Added initialization logic when changing container attributes #162

Added initialization logic when changing container attributes #162

Conversation

devxom commented Dec 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitkarpov commented Dec 26, 2019

NikolayRys commented Dec 26, 2019 • edited Loading

devxom commented Dec 29, 2019

NikolayRys commented Dec 29, 2019

devxom commented Jan 6, 2020

vitkarpov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikolayRys Feb 5, 2020 • 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

NikolayRys Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

devxom commented Jan 7, 2020

vitkarpov commented Jan 9, 2020

devxom commented Jan 9, 2020

vitkarpov commented Jan 10, 2020 • edited Loading

NikolayRys left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikolayRys Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikolayRys Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

NikolayRys Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

NikolayRys commented Mar 25, 2020 • edited Loading

vitkarpov commented Mar 25, 2020

NikolayRys commented Mar 25, 2020

vitkarpov commented Mar 25, 2020

devxom commented Mar 25, 2020 • edited Loading

vitkarpov commented Mar 25, 2020 via email

NikolayRys commented Mar 25, 2020 • edited Loading

devxom commented Mar 27, 2020

NikolayRys commented Apr 29, 2020

devxom commented May 1, 2020

vitkarpov left a comment

Choose a reason for hiding this comment

NikolayRys commented May 5, 2020

devxom commented Dec 25, 2019 •

edited

Loading

NikolayRys commented Dec 26, 2019 •

edited

Loading

NikolayRys Feb 5, 2020 •

edited

Loading

NikolayRys Feb 5, 2020 •

edited

Loading

vitkarpov commented Jan 10, 2020 •

edited

Loading

NikolayRys left a comment •

edited

Loading

NikolayRys Feb 5, 2020 •

edited

Loading

NikolayRys Feb 5, 2020 •

edited

Loading

NikolayRys Feb 11, 2020 •

edited

Loading

NikolayRys commented Mar 25, 2020 •

edited

Loading

devxom commented Mar 25, 2020 •

edited

Loading

NikolayRys commented Mar 25, 2020 •

edited

Loading