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

Fixed #16: Better way to store webpush data in DOM #18

Merged
merged 1 commit into from
Oct 22, 2016

Conversation

arogachev
Copy link
Contributor

Fixed #16: Better way to store webpush data in DOM

Additional notes:

  • Fixed some code style problems in webpush_notifications.py:
    • Removed unused imports.
    • Fixed indendation.
    • Consistency in quotes usage (single quotes).
    • Fixed dictionary formatting.
  • Removed unnecessary spaces around data-group attribute (they already exist around if) in webpush_button.html.
  • Removed unncessary linebreaks in webpush.html.

@safwanrahman
Copy link
Owner

So the fact is that, do everyone will use our webpush button or not. I would be glad if you can find a way to include it in the header.

@arogachev
Copy link
Contributor Author

arogachev commented Oct 10, 2016

I would be glad if you can find a way to include it in the header.

As I said in the issue, including it in header that way is semantically incorrect.

So the fact is that, do everyone will use our webpush button or not.

I think it's premature optimization. Currently extension pretty much forces user to use this button. The current js implementation is very tightly coupled with the button, so user either needs to accept it or rewrite js fully to fit his needs. We can replace data storing element it with hidden input or even leave as div, but what's the point? Everything in current implementation is built on top of this button.

When you decide to make it optional or remove it completely so extension provides only basic subscribing functionality and how to trigger it from interface will be user's decision (should be implemented by him) - that's when you need to worry about it. Along with it you can refactor the way data is passed from server.

And the main goal is to fix incorrect semantics and allow user to register scripts after {% webpush %} tag.

@safwanrahman
Copy link
Owner

Like you explanation. Thanks @arogachev for explaining. I will review the PR later today

@arogachev
Copy link
Contributor Author

@safwanrahman OK. I'll add more issues about JS related problems.

@safwanrahman
Copy link
Owner

Code looks Good. I will check tomorrow in browser then merge it.
Thanks @arogachev for the Pull Request

@safwanrahman
Copy link
Owner

Sorry, could not get time. going to do it tomorrow, Friday (Week end here)

@safwanrahman safwanrahman merged commit c2428f4 into safwanrahman:master Oct 22, 2016
@safwanrahman
Copy link
Owner

Sorry for the delay. It was quite a busy week!
I have tested in and it works without any problem.
So lets merge it!
Thanks @arogachev

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

Successfully merging this pull request may close these issues.

None yet

2 participants