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

[IMP] (website_)mass_mailing, web_editor: Improve newsletter popup #29353

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@kig-odoo
Copy link
Contributor

commented Dec 7, 2018

Improve newsletter website popup.
See sub-commits for details.

Task: 1903256

@robodoo robodoo added the seen 🙂 label Dec 7, 2018

@C3POdoo C3POdoo added the RD label Dec 7, 2018

@robodoo robodoo added the CI 🤖 label Dec 7, 2018

@kig-odoo kig-odoo force-pushed the odoo-dev:master-website-imp-email-popup-kig branch Dec 10, 2018

@robodoo robodoo removed the CI 🤖 label Dec 10, 2018

@kig-odoo kig-odoo force-pushed the odoo-dev:master-website-imp-email-popup-kig branch Dec 10, 2018

@robodoo robodoo added the CI 🤖 label Dec 10, 2018

@kig-odoo kig-odoo force-pushed the odoo-dev:master-website-imp-email-popup-kig branch Dec 17, 2018

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Dec 17, 2018

@kig-odoo kig-odoo force-pushed the odoo-dev:master-website-imp-email-popup-kig branch Dec 24, 2018

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Dec 24, 2018

@kig-odoo kig-odoo force-pushed the odoo-dev:master-website-imp-email-popup-kig branch Dec 24, 2018

@robodoo robodoo removed the CI 🤖 label Dec 24, 2018

@kig-odoo kig-odoo force-pushed the odoo-dev:master-website-imp-email-popup-kig branch Dec 24, 2018

@robodoo robodoo added the CI 🤖 label Dec 24, 2018

@kig-odoo kig-odoo force-pushed the odoo-dev:master-website-imp-email-popup-kig branch Jan 1, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 1, 2019

@kig-odoo kig-odoo force-pushed the odoo-dev:master-website-imp-email-popup-kig branch Jan 2, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 2, 2019

@kig-odoo kig-odoo force-pushed the odoo-dev:master-website-imp-email-popup-kig branch Jan 3, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 3, 2019

@dja-odoo

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

LGTM

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels May 9, 2019

@kig-odoo kig-odoo force-pushed the odoo-dev:master-website-imp-email-popup-kig branch 2 times, most recently from 50d1ec6 to 2415490 May 10, 2019

kig-odoo added a commit to odoo-dev/odoo that referenced this pull request May 10, 2019

[IMP] website_mass_mailing, *: improve newsletter
* website, mass_mailing
This commit adds various improvements in newsletter popup as follow,
- Improves UI for newsletter popup.
- Till now we only have one popup for every website. But with this commit,
 we can have different popup per website (if multi-website activated).
- Before this commit the popup is not appearing in mobile/tablet devices.
 Now in mobile/tablet devices, popup appears automatically after 5 seconds.
- Added ability to change popup background just like a normal snippet.
- Before this commit, user need to enable popup snippet from the settings.
 We removed that settings so now popup snippet will be available by default.
- Added new snippet "Newsletter block".
- Display notification after successful subscription for newsletter snippets.

Task-1903256
closes odoo#29353

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels May 10, 2019

@kig-odoo kig-odoo force-pushed the odoo-dev:master-website-imp-email-popup-kig branch from 7ba0f98 to 21cfe92 May 17, 2019

kig-odoo added a commit to odoo-dev/odoo that referenced this pull request May 17, 2019

[IMP] website_mass_mailing, *: improve newsletter
* website, mass_mailing
This commit adds various improvements in newsletter popup as follow,
- Improves UI for newsletter popup.
- Till now we only have one popup for every website. But with this commit,
 we can have different popup per website (if multi-website activated).
- Before this commit the popup is not appearing in mobile/tablet devices.
 Now in mobile/tablet devices, popup appears automatically after 5 seconds.
- Added ability to change popup background just like a normal snippet.
- Before this commit, user need to enable popup snippet from the settings.
 We removed that settings so now popup snippet will be available by default.
- Added new snippet "Newsletter block".
- Display notification after successful subscription for newsletter snippets.

Task-1903256
closes odoo#29353

@robodoo robodoo removed the CI 🤖 label May 17, 2019

@kig-odoo kig-odoo force-pushed the odoo-dev:master-website-imp-email-popup-kig branch from 21cfe92 to f7a1596 May 17, 2019

kig-odoo added a commit to odoo-dev/odoo that referenced this pull request May 17, 2019

[IMP] website_mass_mailing, *: improve newsletter
* website, mass_mailing

This commit adds various improvements in newsletter popup as follow,
- Improves UI for newsletter popup.
- Till now we only have one popup for every website. But with this commit,
  we can have different popup per website (if multi-website activated).
- Before this commit the popup is not appearing in mobile/tablet devices.
  Now in mobile/tablet devices, popup appears automatically after 5 seconds.
- Added ability to change popup background just like a normal snippet.
- Before this commit, user need to enable popup snippet from the settings.
  We removed that settings so now popup snippet will be available by default.
- Added new snippet "Newsletter block".
- Display notification after successful subscription for newsletter snippets.

Task-1903256
closes odoo#29353

@kig-odoo kig-odoo force-pushed the odoo-dev:master-website-imp-email-popup-kig branch from f7a1596 to 1b90f67 May 17, 2019

kig-odoo added a commit to odoo-dev/odoo that referenced this pull request May 17, 2019

[IMP] website_mass_mailing, *: improve newsletter
* website, mass_mailing

This commit adds various improvements in newsletter snippet as follow,
- Improves UI for newsletter popup.
- Till now we only have one popup for every website. But with this commit,
  we can have different popup per website (if multi-website activated).
- Before this commit the popup is not appearing in mobile/tablet devices.
  Now in mobile/tablet devices, popup appears automatically after 5 seconds.
- Added ability to change popup background just like a normal snippet.
- Before this commit, user need to enable popup snippet from the settings.
  We removed that settings so now popup snippet will be available by default.
- Added new snippet "Newsletter block".
- Display notification after successful subscription for newsletter snippets.

Task-1903256
closes odoo#29353

@robodoo robodoo added the CI 🤖 label May 17, 2019

mass_mailing_list = request.env['mail.mass_mailing.list'].sudo().browse(int(newsletter_id))
data['content'] = mass_mailing_list.popup_content,
data['redirect_url'] = mass_mailing_list.popup_redirect_url
domain = expression.AND([request.website.website_domain(), [('mailing_list_id.id', '=', newsletter_id)]])

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo May 20, 2019

Contributor

You don't need the .id in your expression, this is one of the reasons we name the m2o fields 'xxxx_id' :)

@route(['/website_mass_mailing/set_content'], type='json', website=True, auth="user")
def set_mass_mailing_content(self, newsletter_id, content, **post):
PopupModel = request.env['mail.mass_mailing.popup']
domain = expression.AND([request.website.website_domain(), [('mailing_list_id.id', '=', newsletter_id)]])

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo May 20, 2019

Contributor

Same here



class MassMailingPopup(models.Model):
_name = 'mail.mass_mailing.popup'

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo May 20, 2019

Contributor

@JKE-be Any convention for naming models ? Should it start with "website_mass_mailing" ?

This comment has been minimized.

Copy link
@JKE-be

JKE-be May 22, 2019

Contributor

website.mass_mailing.popup lgtm if this popup is not shown outside of Website

}).then(function (data) {
if (!data.length) {
self.$dialog.find('.btn-primary').prop('disabled', true);
}

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo May 20, 2019

Contributor

Note sure of what case you are handling here, could you explain? Thanks

This comment has been minimized.

Copy link
@kig-odoo

kig-odoo May 22, 2019

Author Contributor

To handle this case see: https://drive.google.com/file/d/14y4z8ENAkvoFuK8Ihp61HWYKBhbXYkQq/view
But strange this code was working at that time but it shouldn't.
BTW I fixed it another way.

// Don't open prompt if input dropped inside newsletter modal
if (this.$target.closest('.o_newsletter_modal').length) {
return;
}

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo May 20, 2019

Contributor

The whole option should not be initialized at all for snippets inside newsletter modals, use data-exclude on the option.

This comment has been minimized.

Copy link
@kig-odoo

kig-odoo May 22, 2019

Author Contributor

yes, we can do this by data-exclude. but this way we don't have any Customize options on the input.so user not able to remove input.
see issue: https://drive.google.com/file/d/1CrA6p0pBaiKQDxN-AJHr8-YiO9hY3fv5/view

to enable Customize option maybe we can change the structure of input snippet by adding
row > col-* not sure it is a good idea or not.

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo May 22, 2019

Contributor

For the remove button to appear there does not need to be any option. It depends on data-drop-near and data-drop-in. What you should do is split the option definition.

<div data-js="my_js" data-selector="A" data-drop-in="IN" data-drop-near="NEAR">
    <a ...></a>
</div>

=>

<t t-set="my_great_selector" t-value="A"/>
<div data-js="my_js" t-att-data-selector="my_great_selector">
    <a ...></a>
</div>
<div t-att-data-selector="my_great_selector" data-drop-in="IN" data-drop-near="NEAR"/>

Then you can add your data-exclude only for the data-js part.

padding: 100px 0px;
text-align: center;
&:before {
content: attr(data-editor-message);

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo May 20, 2019

Contributor

Is not this automatic with oe_structure element ?

This comment has been minimized.

Copy link
@kig-odoo

kig-odoo May 22, 2019

Author Contributor

I tried but it's not working directly on oe_structure so I've added custom CSS for this block. but right now I removed duplicate CSS by adding these two classes o_editable and oe_empty on modal-body in edit mode. also, we need to set the data-editor-message attribute manually.

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo May 22, 2019

Contributor

Yes, it is what you needed to do for the CSS part. By "manually" I suppose you mean with JS? If so, yes that is normal.

}

.modal-body {
padding: 0;

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo May 20, 2019

Contributor

Could use the p-0 class on the modal-body to avoid more css.

This comment has been minimized.

Copy link
@kig-odoo

kig-odoo May 22, 2019

Author Contributor

in the empty block, we are adding padding: 100px 0px; but as p-0 class having !important; it will not apply padding with p-0 class.

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo May 22, 2019

Contributor

Mmh, ok

<t t-name="website_mass_mailing.popup">
<div class="modal fade o_newsletter_modal" t-att-id="modalID" t-attf-data-backdrop="#{isEditMode ? 'false' : 'true'}" role="dialog">
<div class="modal-dialog modal-dialog-centered" role="document">
<div class="modal-content rounded-0">

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo May 20, 2019

Contributor

Even if there are differences, I think using the Dialog class is better. It avoids redefining standard bootstrap XML structure. Would it be possible to create an extension of the Dialog class and alter the modal rendering by using JS on the $modal variable ?

This comment has been minimized.

Copy link
@kig-odoo

kig-odoo May 22, 2019

Author Contributor

With the Dialog class whenever we open dialog it is appended to the body so the content inside the dialog is no longer editable.
Also, we need to do lots of modification in $modal element like removing header and footer, set backdrop attribute and few more.
we need to pass $target When we initialize the Dialog and then we need to override destroy method and set updated content to $target by .data().
To simplify this I've used the standard bootstrap structure.
Correct me if I'm wrong?

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo May 22, 2019

Contributor

You can add new options to the Dialog class directly

  • Where it should be appended (default to body)
  • Whether a header / footer should be display
  • If we need a backdrop or not (default to true)
  • ...

The code may not be simpler but it would not duplicate the modal structure and it would be reusable for every dialog. What do you think ?

This comment has been minimized.

Copy link
@kig-odoo

kig-odoo May 23, 2019

Author Contributor

I've found that the bootstrap is appending model only in the body see:
https://github.com/odoo/odoo/blob/master/addons/web/static/lib/bootstrap/js/modal.js#L283
in this commit, I've added option via monkey patch to append modal to different target
odoo-dev@ae708e5
can you give your thoughts on this? :|

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo May 23, 2019

Contributor

Your link shows that BS is only appending the modal in the body if it is not already in the DOM. I think that if you just add

this.$modal.appendTo($parentNode);

before using .modal('show'), it should work. Is this not the case ?

<group groups="website_mass_mailing.group_website_popup_on_exit">
<field name="popup_redirect_url"/>
<group>
<field name="toast_content"/>

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo May 20, 2019

Contributor

You are positioning a group with field inside a div which has the oe_title class... seems strange to me :)

<div class="col-12 col-md-6 pt112 pb112 o_mm_img_block oe_img_bg oe_custom_bg" style='background-image: url("/web/image/website.library_image_17");'>
<p>&amp;nbsp;</p>
</div>
<div class="col-12 col-md-6 pt112 pb112 o_mm_content_block">

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo May 20, 2019

Contributor

The mm_content_block class can be removed

This comment has been minimized.

Copy link
@kig-odoo

kig-odoo May 22, 2019

Author Contributor

odoo-dev@af9de05#diff-34ce1bd1849e1ffa6c277d5b87db7822R48 Here I pass this class as a selector so that the user can set background for this block.

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo May 22, 2019

Contributor

ok

@kig-odoo kig-odoo force-pushed the odoo-dev:master-website-imp-email-popup-kig branch from 1b90f67 to 173c5a5 May 21, 2019

kig-odoo added a commit to odoo-dev/odoo that referenced this pull request May 21, 2019

[IMP] website_mass_mailing, *: improve newsletter
* website, mass_mailing

This commit adds various improvements in newsletter snippet as follow,
- Improves UI for newsletter popup.
- Till now we only have one popup for every website. But with this commit,
  we can have different popup per website (if multi-website activated).
- Before this commit the popup is not appearing in mobile/tablet devices.
  Now in mobile/tablet devices, popup appears automatically after 5 seconds.
- Added ability to change popup background just like a normal snippet.
- Before this commit, user need to enable popup snippet from the settings.
  We removed that settings so now popup snippet will be available by default.
- Added new snippet "Newsletter block".
- Display notification after successful subscription for newsletter snippets.

Task-1903256
closes odoo#29353

@robodoo robodoo removed the CI 🤖 label May 21, 2019

@kig-odoo kig-odoo force-pushed the odoo-dev:master-website-imp-email-popup-kig branch from 173c5a5 to af9de05 May 21, 2019

kig-odoo added a commit to odoo-dev/odoo that referenced this pull request May 21, 2019

[IMP] website_mass_mailing, *: improve newsletter
* website, mass_mailing

This commit adds various improvements in newsletter snippet as follow,
- Improves UI for newsletter popup.
- Till now we only have one popup for every website. But with this commit,
  we can have different popup per website (if multi-website activated).
- Before this commit the popup is not appearing in mobile/tablet devices.
  Now in mobile/tablet devices, popup appears automatically after 5 seconds.
- Added ability to change popup background just like a normal snippet.
- Before this commit, user need to enable popup snippet from the settings.
  We removed that settings so now popup snippet will be available by default.
- Added new snippet "Newsletter block".
- Display notification after successful subscription for newsletter snippets.

Task-1903256
closes odoo#29353

@robodoo robodoo added the CI 🤖 label May 21, 2019

[IMP] website_mass_mailing, *: improve newsletter
* website, mass_mailing

This commit adds various improvements in newsletter snippet as follow,
- Improves UI for newsletter popup.
- Till now we only have one popup for every website. But with this commit,
  we can have different popup per website (if multi-website activated).
- Before this commit the popup is not appearing in mobile/tablet devices.
  Now in mobile/tablet devices, popup appears automatically after 5 seconds.
- Added ability to change popup background just like a normal snippet.
- Before this commit, user need to enable popup snippet from the settings.
  We removed that settings so now popup snippet will be available by default.
- Added new snippet "Newsletter block".
- Display notification after successful subscription for newsletter snippets.

Task-1903256
closes #29353

@kig-odoo kig-odoo force-pushed the odoo-dev:master-website-imp-email-popup-kig branch from af9de05 to f95fb29 May 22, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels May 22, 2019

IMP

@robodoo robodoo removed the CI 🤖 label May 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.