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

Master ui for archived followers jgi #32084

Closed

Conversation

Projects
None yet
4 participants
@jgi-odoo
Copy link

commented Mar 25, 2019

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo robodoo added the CI 🤖 label Mar 25, 2019

@alexkuhn alexkuhn requested review from alexkuhn and Xavier-Do Mar 25, 2019

@alexkuhn alexkuhn added the RD label Mar 25, 2019

@@ -36,12 +36,12 @@
-->
<t t-name="mail.Followers.partner">
<div role="menuitem" class="o_partner">
<a class="dropdown-item o_mail_redirect"
<a t-attf-class="dropdown-item o_mail_redirect {{ record.active === false ? 'o_inactive_partner': '' }}"

This comment has been minimized.

Copy link
@alexkuhn

alexkuhn Mar 25, 2019

Contributor

record.active === false => !record.active

@@ -110,6 +114,9 @@
@include o-position-absolute(6px, 18px);
cursor: pointer;
}
.o_inactive_partner_img{

This comment has been minimized.

Copy link
@alexkuhn

alexkuhn Mar 25, 2019

Contributor

best to use > when you can, also space before {

@@ -97,6 +97,10 @@
text-overflow: ellipsis;
height: $o-mail-partner-avatar-size;
padding: 3px $o-mail-partner-avatar-size + 8px;
&.o_inactive_partner {

This comment has been minimized.

Copy link
@alexkuhn

alexkuhn Mar 25, 2019

Contributor

> + empty newline before

This comment has been minimized.

Copy link
@alexkuhn

alexkuhn Mar 25, 2019

Contributor

oops, no > here, but still newline before

session: {partner_id: 1},
});

assert.strictEqual(form.$('.o_inactive_partner').length, 1, 'there is 1 inactive follower');

This comment has been minimized.

Copy link
@alexkuhn

alexkuhn Mar 25, 2019

Contributor

assert.containsOnce() or assert.isVisible()

});

assert.strictEqual(form.$('.o_inactive_partner').length, 1, 'there is 1 inactive follower');
assert.strictEqual(form.$('.o_inactive_partner_img').length, 1, 'there is 1 inactive follower\'s avatar');

This comment has been minimized.

Copy link
@alexkuhn

alexkuhn Mar 25, 2019

Contributor

Would be much better to assert on each follower list item. Could use [data-oe-id="<id>"] in the jQuery selector.

active: false,
}];

var def;

This comment has been minimized.

Copy link
@alexkuhn

alexkuhn Mar 25, 2019

Contributor

unused

id: 1,
name: "Admin",
email: "admin@example.com",
res_id: resID,

This comment has been minimized.

Copy link
@alexkuhn

alexkuhn Mar 25, 2019

Contributor

does not really make sense to have same res_id for all followers.

if (route === '/mail/read_followers') {
return Promise.resolve(def).then(function () {
return {
followers: _.filter(followers, function (follower) {

This comment has been minimized.

Copy link
@alexkuhn

alexkuhn Mar 25, 2019

Contributor

This looks basically the server-side behaviour of this method. Would be nice to define this in mail/mock_server, so that it does that by default when there are followers in this.data. (You may need to adapt some other tests in this file)

This comment has been minimized.

Copy link
@alexkuhn

alexkuhn Mar 25, 2019

Contributor

to do with read_followers refactoring, so let's keep it that way for the moment

@@ -110,6 +114,9 @@
@include o-position-absolute(6px, 18px);
cursor: pointer;
}
.o_inactive_partner_img{

This comment has been minimized.

Copy link
@Xavier-Do

Xavier-Do Mar 25, 2019

Contributor

This should be in the o_partner section too

href="#"
t-att-title="record.name"
t-att-data-oe-model="record.res_model"
t-att-data-oe-id="record.res_id"><t t-esc="record.name"/></a>
<img t-att-src="record.avatar_url" alt="Avatar"/>
<img t-att-src="record.avatar_url" alt="Avatar" t-attf-class="{{ record.active === false ? 'o_inactive_partner_img': '' }}"/>

This comment has been minimized.

Copy link
@Xavier-Do

Xavier-Do Mar 25, 2019

Contributor

I wonder if it wouldn't be cleaner to add a class on div.o_partner and adapt the css accordingly since you are using this condition twice. in the same dom element

@robodoo robodoo removed the CI 🤖 label Mar 26, 2019

@@ -2307,7 +2306,7 @@ QUnit.test('followers widget: display inactive followers with a different style'
'</form>',
mockRPC: function (route, args) {
if (route === '/mail/read_followers') {
return Promise.resolve(def).then(function () {
return Promise.resolve().then(function () {

This comment has been minimized.

Copy link
@alexkuhn

alexkuhn Mar 26, 2019

Contributor
return Promise.resolve({
   /* ... */
});

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 26, 2019

@jgi-odoo jgi-odoo force-pushed the odoo-dev:master-ui-for-archived-followers-jgi branch from 4ae4b1c to 3b2e640 Mar 26, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 26, 2019

[IMP] mail: add a different layout for inactive followers
When displaying the followers of a document, followers who are set as inactive will be displayed in a different layout than the active ones. The purpose is to mark a clear difference between active and inactive followers.

Task #1957849

@jgi-odoo jgi-odoo force-pushed the odoo-dev:master-ui-for-archived-followers-jgi branch from 3b2e640 to a5b8ff7 Mar 27, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 27, 2019

@Xavier-Do

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

robodoo r+

@robodoo robodoo added the r+ 👌 label Apr 2, 2019

robodoo pushed a commit that referenced this pull request Apr 2, 2019

[IMP] mail: add a different layout for inactive followers
When displaying the followers of a document, followers who are set as inactive will be displayed in a different layout than the active ones. The purpose is to mark a clear difference between active and inactive followers.

Task #1957849

closes #32084

Signed-off-by: Xavier Dollé (xdo) <xdo@odoo.com>
@robodoo

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

Merged, thanks!

@robodoo robodoo closed this Apr 2, 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.