Issue #139 - Convert ENS Address To Link functionality #140

Samyoul · 2017-10-24T17:15:45Z

Resolves #139

I've refactored the regex portions of the application so that we only instantiate the RegEx once and abstracted away the conversion logic into a more generic function

Samyoul · 2017-10-24T17:17:00Z

js/DomManipulator.js

-
-        var arrWhitelistedTags = new Array("code", "span", "p", "td", "li", "em", "i", "b", "strong", "small");
-        var strRegex = /(^|\s|:|-)((?:0x)[0-9a-fA-F]{40})(?:\s|$)/gi;
+        var arrWhitelistedTags = ["code", "span", "p", "td", "li", "em", "i", "b", "strong", "small"];


I've also refactored the array instantiation, let me know if this is a problem.

It's all good :)

409H · 2017-10-24T20:07:55Z

Awesome - testing now!

Samyoul · 2017-10-24T20:17:43Z

Ah, I've just spotted that this will fail with a few of the block explorers. I need to handle that.

409H

I think we may have to modify the regex a little more... I'm doing some tests here - feel free to add some more and experiment.

https://regex101.com/r/feqr8J/1

409H · 2017-10-24T21:11:48Z

@Samyoul can you hop on to MEW slack channel or join me over at https://tlk.io/b383fb if you're still working on this tonight

Samyoul · 2017-10-25T13:03:52Z

We settled on https://regex101.com/r/feqr8J/7

(\s|^|\.|\"|\'|\>)([a-z0-9][a-z0-9-\.]+[a-z0-9](?:.eth))(\s|$|\.|\"|\'|\<)

With the following substitution string:

$1<a title="See this address on the blockchain explorer" href="https://etherscan.io/address/$2" class="ext-etheraddresslookup-link">$2</a>$3

This handles standard domain structure.

Additionally refactored the constructor so that search and replace settings are handled by a single method. Moved replace pattern into an abstracted pattern variable to allow for multiple patterns to be used. TODO: Check for explorers supporting ENS, on fail notify user.

Samyoul · 2017-10-25T13:53:45Z

Working on checking for supported block explorers, on failure give warning.

Quite a lot of refactor work was done on this commit, and I should have just commited each part separately. But I wanted to fix the bug I found because it was messing up my testing. Bug Fix : basically stopped regex being applied to anything that wasn't a textNode, otherwise we would have attribute overwrites and other kinds of mess. In order for this to work I've created a tempory "slot" element that holds new elements, after regex is applied they are moved into the parent node. Added docblocks

Samyoul · 2017-10-27T21:24:08Z

I've committed the functionality that completes this issue. However I'm sorry that so much refactor work was push in a single commit. Notes from my commit :

Quite a lot of refactor work was done on this commit, and I should have just commited each part separately. But I wanted to fix the bug I found because it was messing up my testing.

Bug Fix : basically stopped regex being applied to anything that wasn't a textNode, otherwise we would have attribute overwrites and other kinds of mess. In order for this to work I've created a tempory "slot" element that holds new elements, after regex is applied they are moved into the parent node after temporary slot is removed.

Added docblocks

Samyoul · 2017-10-27T21:25:40Z

js/DomManipulator.js

    init()
    {
        let objBrowser = chrome ? chrome : browser;
        //Get the highlight option for the user
        objBrowser.runtime.sendMessage({func: "highlight_option"}, function(objResponse) {
            if(objResponse && objResponse.hasOwnProperty("resp")) {
-                this.blHighlight = (objResponse.resp == 1 ? true : false);
+                this.blHighlight = (objResponse.resp == 1);


I've changed this because it is basically the thing:

(objResponse.resp == 1 ? true : false) == (objResponse.resp == 1)

Aha, nice spot! 👍

Samyoul · 2017-10-27T21:33:20Z

options.html

        <option value="https://ethplorer.io/address">Ethplorer.io</option>
        <option value="http://ethergraphs.com/dashboard">Ethergraphs.com</option>
    </select>
+    <div>
+        <p>* ENS address compatible.</p>
+    </div>


Made this amendment to allow the user to know which explorers will allow them the ability view ENS addresses. We can in future add new functionality to lookup the underlying Ethereum address then link to this on the user's chosen explorer.

Can we make this text smaller please?
Just a small UI change, put it in <small></small> tags. (I have selective OCD 😕)

Or do you have an idea on how to present notes/not-main-labels on the UI?

I'll make this smaller for now, we can discuss ideas later, though UI isn't really my strong suit.

Thanks! Once v1.9 is up, we can arrange a more in-depth discussion on it for later this week when both our schedules are free.

Samyoul · 2017-10-27T22:22:06Z

I've spotted a bug with this, I need to take a look at it:

In some cases the new .eth address is moved to the end of the element.

Likely related to :

https://github.com/409H/EtherAddressLookup/pull/140/files#diff-082dfc69c6b55d30d3aef681bb1b3fe8R232

tidyUpSlots()
 {
    var slots = document.querySelectorAll("slot.ext-etheraddresslookup-temporary");
    for(var i=0; i < slots.length; i++){
        while(slots[i].childNodes.length > 0){
           slots[i].parentNode.appendChild(slots[i].firstChild); // <-- Problem here.
        }
        slots[i].parentNode.removeChild(slots[i]);
    }
}

Samyoul · 2017-10-28T08:57:25Z

Bug is resolved. Ready for testing.

409H · 2017-10-29T16:33:12Z

@Samyoul Looks all good here! Awesome work. Please can you adjust the version value to 1.9 in manifest.json ?

Samyoul · 2017-10-29T19:22:59Z

@409H No problem. Done.

409H · 2017-10-30T11:15:11Z

@Samyoul I will merge and package to the Chrome store tonight! 🎉

Thanks for all your work towards this issue!

Samyoul · 2017-10-30T15:40:50Z

"Small" change made 😆

Samyoul added 2 commits October 24, 2017 16:35

Update to .gitignore

a699c31

Added ENS address highlighting and linking

55e3fef

Samyoul commented Oct 24, 2017

View reviewed changes

409H changed the title ~~Issue/139~~ Issue #139 - Convert ENS Address To Link functionality Oct 24, 2017

409H added the enhancement label Oct 24, 2017

409H self-requested a review October 24, 2017 20:12

409H reviewed Oct 24, 2017

View reviewed changes

409H changed the title ~~Issue #139 - Convert ENS Address To Link functionality~~ WIP: Issue #139 - Convert ENS Address To Link functionality Oct 25, 2017

Samyoul commented Oct 27, 2017

View reviewed changes

Samyoul mentioned this pull request Oct 27, 2017

Using ENS Lookup to get underlying Ethereum Address #143

Open

Fixed Node positioning bug

ff98107

409H changed the title ~~WIP: Issue #139 - Convert ENS Address To Link functionality~~ Issue #139 - Convert ENS Address To Link functionality Oct 29, 2017

409H added the ready-to-merge label Oct 29, 2017

Updated application verion

70904f6

Samyoul mentioned this pull request Oct 30, 2017

Unit Test - DOM Manipulation #151

Open

Small change to UI

311f014

409H merged commit 611b7e4 into 409H:master Oct 30, 2017

409H removed the ready-to-merge label Nov 1, 2017

Samyoul deleted the issue/139 branch November 7, 2017 11:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #139 - Convert ENS Address To Link functionality #140

Issue #139 - Convert ENS Address To Link functionality #140

Samyoul commented Oct 24, 2017

Samyoul Oct 24, 2017

409H Oct 27, 2017

409H commented Oct 24, 2017 •

edited

Samyoul commented Oct 24, 2017

409H left a comment

409H commented Oct 24, 2017

Samyoul commented Oct 25, 2017

Samyoul commented Oct 25, 2017

Samyoul commented Oct 27, 2017

Samyoul Oct 27, 2017

409H Oct 27, 2017

Samyoul Oct 27, 2017

409H Oct 27, 2017

409H Oct 30, 2017 •

edited

Samyoul Oct 30, 2017

409H Oct 30, 2017

Samyoul commented Oct 27, 2017

Samyoul commented Oct 28, 2017

409H commented Oct 29, 2017

Samyoul commented Oct 29, 2017

409H commented Oct 30, 2017 •

edited

Samyoul commented Oct 30, 2017

Issue #139 - Convert ENS Address To Link functionality #140

Issue #139 - Convert ENS Address To Link functionality #140

Conversation

Samyoul commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

409H commented Oct 24, 2017 • edited

Samyoul commented Oct 24, 2017

409H left a comment

Choose a reason for hiding this comment

409H commented Oct 24, 2017

Samyoul commented Oct 25, 2017

Samyoul commented Oct 25, 2017

Samyoul commented Oct 27, 2017

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

409H Oct 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Samyoul commented Oct 27, 2017

Samyoul commented Oct 28, 2017

409H commented Oct 29, 2017

Samyoul commented Oct 29, 2017

409H commented Oct 30, 2017 • edited

Samyoul commented Oct 30, 2017

409H commented Oct 24, 2017 •

edited

409H Oct 30, 2017 •

edited

409H commented Oct 30, 2017 •

edited