Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QR code tag #41

Closed
fwenzel opened this issue Oct 15, 2013 · 17 comments
Closed

QR code tag #41

fwenzel opened this issue Oct 15, 2013 · 17 comments

Comments

@fwenzel
Copy link

fwenzel commented Oct 15, 2013

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
Copy link
Member

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
Copy link

arasbm commented Oct 16, 2013

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

@csuwildcat
Copy link
Member

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//issues/41#issuecomment-26390397
.

@arasbm
Copy link

arasbm commented Oct 16, 2013

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
Copy link
Member

@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:

  • QR Codes can be more than just links - letting developers put it in the tag itself is cleaner than pushing a big chunk of HTML into an attribute string.
  • innerHTML changes are observable - we provide this functionality in X-Tag via the addObserver and removeObserver methods

@fwenzel
Copy link
Author

fwenzel commented Oct 17, 2013

Nice work drafting this, guys!

@arasbm
Copy link

arasbm commented Oct 18, 2013

@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
Copy link

arasbm commented Oct 21, 2013

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
Copy link
Member

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//issues/41#issuecomment-26574418
.

@fwenzel
Copy link
Author

fwenzel commented Oct 21, 2013

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
Copy link
Member

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//issues/41#issuecomment-26735323
.

@pennyfx
Copy link
Member

pennyfx commented Oct 21, 2013

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//issues/41#issuecomment-26737465
.

@fwenzel
Copy link
Author

fwenzel commented Oct 21, 2013

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

@arasbm
Copy link

arasbm commented Oct 21, 2013

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

@pennyfx
Copy link
Member

pennyfx commented Oct 21, 2013

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

@arasbm
Copy link

arasbm commented Oct 23, 2013

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants