x-tag / core

The Heart of X-Tag
http://x-tag.github.io/
Other
1.25k stars 151 forks source link

QR code tag #41

Closed fwenzel closed 10 years ago

fwenzel commented 10 years ago

QR codes are everywhere (though their UX is questionable). It would be splendid to have a tag:

<x-qr>http://mozilla.org</x-qr>

and it appears.

That would also enable QR codes to be accessible. How hot is that.

csuwildcat commented 10 years ago

Basically all they need to do is wrap this JS lib up and have the tag refresh when its innerHTML changes: http://davidshimjs.github.io/qrcodejs/

arasbm commented 10 years ago

That sounds like something I could do. Can I take this task?

csuwildcat commented 10 years ago

Heck yes! We'd love to have you contribute. I'll send you a developer best practices guide shortly ;) On Oct 15, 2013 8:26 PM, "Aras Balali Moghaddam" notifications@github.com wrote:

That sounds like something I could do. Can I take this task?

— Reply to this email directly or view it on GitHubhttps://github.com/x-tag/core/issues/41#issuecomment-26390397 .

arasbm commented 10 years ago

Awesome! Looking forward to see that guide.

I have a question though. Should we use innerHTML as the link or define an attribute such as href for the link. I ask because when I looked at existing x-tag components they seem to heavily make use of getters and setters. It make sense to me because it carries a bit more semantic with it.

Another potential issue with using innerHTML is listening to changes in the link. For example if x-qr innerHTML changes using Javascript, we want to subscribe to that change and update the UI. But to update the UI we are probably going to update the innerHTML content. Is there a way around this loop?

However, maybe there are appealing reasons for using the innerHTML instead? Which do you think would work better?

csuwildcat commented 10 years ago

@arasbm here's the guide we've been developing in partnership with the Google folks: https://docs.google.com/document/d/1lbWrU0qsGMijDwzYNttUw352lUhfU7DJW4yxA_A8hq4/edit?usp=sharing

As for this tag, the right move here is to use innerHTML for the following two reasons:

fwenzel commented 10 years ago

Nice work drafting this, guys!

arasbm commented 10 years ago

@csuwildcat ok that make sense, innerHTML it will be then. Do you have an example for using addObserver? I couldn’t find it in the docs: http://www.x-tags.org/docs I tried this code but it dies not seem to work:

     xtag.addObserver(element, function() {
       alert('you changed the innerHTML');
     });

I tested it by selecting the element and changing its innerHTML in console.

Thanks for the guide by the way, it is very useful.

arasbm commented 10 years ago

just an update that I have started implementing this tag here: https://github.com/arasbm/x-qr It is VERY preliminary and need lots of improvements, but it works.

I have not been able to figure out a reasonable way to use the innerHTML yet, instead I am using the whole elements.attributes to pass all options straight down to the qrcode lib (bad idea?). Here is how you can use the tag right now:

<x-qr id="xqr" text="arasbm.com" width=500 height=500 
  color-light="#FCFFCD" color-dark="#65002D">text content</x-qr>

the code is generated from the text attribute, and the text content inside the element is not special. Let me know what you think about this interface. I could not get the event handler on innerHTML working, and even if I do, I am not exactly sure how to distinguish the qrcode element itself from the innerHTML content. Because right now the code itself lives visibly inside the x-qr tag. I am sure this is due to my lack of knowledge about x-tag, so any advice would be appreciated.

@fwenzel thanks for this awesome idea by the way!

csuwildcat commented 10 years ago

The addObserver() method takes 3 parameters: element, activity, and function - you're not passing 'inserted' or 'removed' as a second parameter to tell it what you're listening for. On Oct 17, 2013 11:20 PM, "Aras Balali Moghaddam" notifications@github.com wrote:

@csuwildcat https://github.com/csuwildcat ok that make sense, innerHTMLit will be then. Do you have an example for using addObserver? I couldn’t find it in the docs: http://www.x-tags.org/docs I tried this code but it dies not seem to work:

 xtag.addObserver(element, function() {
   alert('you changed the innerHTML');
 });

I tested it by selecting the element and changing its innerHTML in console.

Thanks for the guide by the way, it is very useful.

— Reply to this email directly or view it on GitHubhttps://github.com/x-tag/core/issues/41#issuecomment-26574418 .

fwenzel commented 10 years ago

I think passing the attributes straight down works, unless the tag has options of its own that you don't want to pass, but you could always handle those first and then pass on the rest of the bunch.

As for text="...", that's the only thing I'd change by instead using the text content of the tag.

Finally, sane defaults are key; <x-qr>hello</x-qr> should do something acceptable. The colors are easy (black/white) but coming up with a default size might need some experimentation. That's the option I imagine will need to be adjusted by devs almost always.

csuwildcat commented 10 years ago

Attributes are fine for the options, but the tag content (innerHTML) needs to be what we use for the actual QR code payload because it would be very ugly to have people paste strings of HTML into an attribute.

To observe these elements you would do: xtag.addObserver(this, 'inserted', function(){ ... }) and xtag.addObserver(this, 'removed', function(){ ... }), the functions here should call a refresh method to repopulate the element with a QR code that matches the new content.

On Mon, Oct 21, 2013 at 9:59 AM, Fred Wenzel notifications@github.comwrote:

I think passing the attributes straight down works, unless the tag has options of its own that you don't want to pass, but you could always handle those first and then pass on the rest of the bunch.

As for text="...", that's the only thing I'd change by instead using the text content of the tag.

Finally, sane defaults are key; hello should do something acceptable. The colors are easy (black/white) but coming up with a default size might need some experimentation. That's the option I imagine will need to be adjusted by devs almost always.

— Reply to this email directly or view it on GitHubhttps://github.com/x-tag/core/issues/41#issuecomment-26735323 .

pennyfx commented 10 years ago

I didn't think that putting HTML in the body was a very likely use case until I looked up vcards. http://en.wikipedia.org/wiki/VCard http://www.moongate.ro/en/products/qr_code-vcard/

On Mon, Oct 21, 2013 at 10:23 AM, Daniel Buchner notifications@github.comwrote:

Attributes are fine for the options, but the tag content (innerHTML) needs to be what we use for the actual QR code payload because it would be very ugly to have people paste strings of HTML into an attribute.

To observe these elements you would do: xtag.addObserver(this, 'inserted', function(){ ... }) and xtag.addObserver(this, 'removed', function(){ ... }), the functions here should call a refresh method to repopulate the element with a QR code that matches the new content.

On Mon, Oct 21, 2013 at 9:59 AM, Fred Wenzel notifications@github.comwrote:

I think passing the attributes straight down works, unless the tag has options of its own that you don't want to pass, but you could always handle those first and then pass on the rest of the bunch.

As for text="...", that's the only thing I'd change by instead using the text content of the tag.

Finally, sane defaults are key; hello should do something acceptable. The colors are easy (black/white) but coming up with a default size might need some experimentation. That's the option I imagine will need to be adjusted by devs almost always.

— Reply to this email directly or view it on GitHub< https://github.com/x-tag/core/issues/41#issuecomment-26735323> .

— Reply to this email directly or view it on GitHubhttps://github.com/x-tag/core/issues/41#issuecomment-26737465 .

fwenzel commented 10 years ago

vcards are not actually HTML though? It's a plain text format?

arasbm commented 10 years ago

thanks for all the feedback! I am travelling today, but i will give this another shot tomorrow with innerHTML

pennyfx commented 10 years ago

@fwenzel I was looking at the xCard, but also thinking about xml data.

arasbm commented 10 years ago

I managed to change the code to use innerHTML. The remaining options are set by attributes. Feel free to checkout the code. Any suggestion/feature request please feel free to open an issue. I have created a couple of issues for questions I have:

This is my first x-tag, so any advice will be highly appreciated :)

csuwildcat commented 10 years ago

I'd like to move this off of the Core repo, as it is not a library issue/bug.