Skip to content

Conditional rendering #79

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

Merged
merged 3 commits into from
May 31, 2017
Merged

Conditional rendering #79

merged 3 commits into from
May 31, 2017

Conversation

neocotic
Copy link
Owner

This PR adds conditional rendering for the image element (and support for future renderers). The image element is now only rendered if it was passed to the QRious constructor as the element option OR if the QRious#image field is referenced at any time (even if it's not used, we can't tell the difference). The canvas element is still always rendered as image depends on it.

Not only will this help with possible Content Security Policy (CSP) issues (see #71) but I should think that it will also improve performance for those only using canvas elements as the browser will no longer spend time generating a data URI for the image element.

To summarise, the image element will only be rendered in the following situations:

const qr = new QRious({
  element: document.createElement('img'),
  value: 'https://github.com/neocotic/qrious'
});
// OR:
const qr = new QRious({
  value: 'https://github.com/neocotic/qrious'
});
qr.image;

@neocotic neocotic added this to the 2.3.0 milestone May 31, 2017
@neocotic neocotic self-assigned this May 31, 2017
@neocotic neocotic changed the base branch from master to develop May 31, 2017 10:48
@neocotic
Copy link
Owner Author

@anthonyryan1 Can you please take a look at these changes to see what you think. This should hopefully resolve #71.

@neocotic
Copy link
Owner Author

@anthonyryan1 Once you've completed your review, please let me know and I can get the final task for 2.3.0 completed quickly and the version released ASAP.

@neocotic
Copy link
Owner Author

@anthonyryan1 I'm just going to go ahead and merge this. Due to lack of tests for this project, I think the best way of checking this out is getting it released and in front of developers.

If you do happen to find anything, we can raise a bug/enhancement to get it fixed/changed in a fix release.

I don't mean to be impatient, I'm actually just enthusiastic about this project again so I won't to get it into a good state so that I can start addressing some of the more fundamental issues (e.g. support for non-ASCII characters).

@neocotic neocotic merged commit eb38a55 into develop May 31, 2017
@neocotic neocotic deleted the conditional-rendering branch May 31, 2017 11:49
@anthonyryan1
Copy link
Contributor

Sorry about the delayed response, I had a chance to review this tonight and it works great!

Great work, and thank you for considering this use case. 😄

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

Successfully merging this pull request may close these issues.

2 participants