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

[Select a Media] Cleaning #28025

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@kea14
Contributor

kea14 commented Oct 22, 2018

task-1896705

@kea14 kea14 requested a review from qsm-odoo Oct 22, 2018

@kea14

This comment has been minimized.

Contributor

kea14 commented Oct 22, 2018

@qsm-odoo Just created this PR, of course still working on it.

@robodoo robodoo added the seen 🙂 label Oct 22, 2018

@C3POdoo C3POdoo added the RD label Oct 23, 2018

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Oct 23, 2018

@qsm-odoo

I know this is work-in-progress but here are some comments to guide you :)

_loadMoreUnsplash: function () {
this.IMAGES_UNSPLASH_ROWS = this.IMAGES_UNSPLASH_ROWS + 2;
this.IMAGES_UNSPLASH_PER_PAGE = this.IMAGES_UNSPLASH_ROWS * this.IMAGES_UNSPLASH_PER_ROW;
this._renderImages();

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 5, 2018

Contributor

Careful: no reference to unsplash in the web_editor module ! Everything which concerns unsplash should be in the web_unsplash module 😉

function nameButton(name) {
if (name === 'document') {
name = "Add a document URL";

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 5, 2018

Contributor

Do not forget that this should be translated (check _t)

This comment has been minimized.

@kea14

kea14 Nov 5, 2018

Contributor

Adding _t('...') is enough ?

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 5, 2018

Contributor

Yes, if you require _t at the top of the file.

Show resolved Hide resolved addons/web_editor/static/src/js/widgets/widgets.js
@@ -81,7 +69,7 @@
<!-- Image choosing part of the Media Dialog -->
<t t-name="web_editor.dialog.image">
<form class="form-inline"
<form class="form-inline card"

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 5, 2018

Contributor

Pretty sure a form-inline card with a container inside and no card-body is not valid bootstrap 😉 https://getbootstrap.com/docs/4.1/getting-started/introduction/ :)

@kea14

This comment has been minimized.

Contributor

kea14 commented Nov 6, 2018

@qsm-odoo
Do you know if it is conceivable in the _uploadImage function (in widgets.js - ImageWidget) to do something like that : Instead of closing modal when a new image is updated, we could get directly the new image next to the other ones without closing modal (and for instance having a green border around it). What do you think ?
(branch updated)

@qsm-odoo

This comment has been minimized.

Contributor

qsm-odoo commented Nov 6, 2018

@kea14 This is a functional question 😉 But know that what you want was the case at some point and was considered a bug that I had to fix ^^

@kea14

This comment has been minimized.

Contributor

kea14 commented Nov 6, 2018

@qsm-odoo Of course this is a functional question and I prefer having your opinion before continuing discussion with po 😄 Thanks for letting me know then :)

@qsm-odoo

@kea14 Time to have a finished task here.

  • I put some comments in the code, could you check and adapt ?
  • Also, could you install a linter as explained here: https://github.com/odoo/odoo/wiki/Javascript-coding-guidelines ?
  • Simplify your work, I doubt there should be an addition of 300+ lines for this task as it is about modifying existing features
  • Rebase your work, your branch is hundreds of commits behind master :)
@@ -34,7 +34,7 @@ Dialog = Dialog.extend({
options = options || {};
this._super(parent, _.extend({}, {
buttons: [
{text: options.save_text || _t("Save"), classes: 'btn-primary', click: this.save},
{text: options.save_text || _t("Add"), classes: 'btn-primary', click: this.save},

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 13, 2018

Contributor

I doubt that the default text for all dialog primary buttons should become "Add" :/

This comment has been minimized.

@kea14

kea14 Nov 13, 2018

Contributor

Then I'll leave 'Save' as this is nonsense to do that

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 13, 2018

Contributor

The point is not to leave "Save". It is to use "Add", only for the media dialog not all dialogs of Odoo.

@@ -194,10 +194,14 @@ var ImageWidget = MediaWidget.extend({
'click .existing-attachments [data-src]': '_onImageClick',
'dblclick .existing-attachments [data-src]': '_onImageDblClick',
'click .o_existing_attachment_remove': '_onRemoveClick',
'click .o_load_more_images': '_onLoadMore',
'keyup #imageurl': '_onImageURL',

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 13, 2018

Contributor

_onImageURL does not mean anything to me, ImageURL is not a verb/event. Please use meaningful names, here I suppose it is an input, so _onImageURLInput and the event would not be keyup but input.

This comment has been minimized.

@kea14

kea14 Nov 13, 2018

Contributor

Already modifed but I agree on meaningful names

Show resolved Hide resolved addons/web_editor/static/src/js/widgets/widgets.js
* @private
*/
_loadMoreImages: function () {
this.IMAGES_ROWS = this.IMAGES_ROWS + 2;

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 13, 2018

Contributor

+= 2

} else {
this.$('.o_load_more_images').removeClass('d-none');
this.$('.o_load_done_msg').addClass('d-none');
}

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 13, 2018

Contributor

Code to factorize here.

var regex = /^(http:\/\/www\.|https:\/\/www\.|http:\/\/|https:\/\/)?[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(:[0-9]{1,5})?(\/.*)?$/;
if (url.match(regex)) {
for (var i in format) {

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 13, 2018

Contributor

Use _.each

Show resolved Hide resolved addons/web_editor/static/src/js/widgets/widgets.js Outdated
/**
* @private
*/
_hideUnsplash: function () {

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 13, 2018

Contributor

Still unsplash here

This comment has been minimized.

@kea14

kea14 Nov 13, 2018

Contributor

hideUnsplash in widgets.js as the purpose is to show Image related stuff and vice versa (hideImage in unsplash_image_widget.js)..

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 13, 2018

Contributor

No... I do not know all your specs and code but there should not be a reference to unsplash here. I suppose you must do something like "showImages" and override it in unsplash to hideUnsplash there.

This comment has been minimized.

@kea14

kea14 Nov 13, 2018

Contributor

Yep, ok then :)

views: prevented[id]
}));
});
if (confirm(_t('Are you sure to delete this component ?'))) {

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 13, 2018

Contributor

Check Dialog.confirm

This comment has been minimized.

@kea14

kea14 Nov 15, 2018

Contributor

I don't get it, require('web.Dialog') at the top of the file and using Dialog.confirm(..) isn't enough ?

// Empty unsplash search field
this.$('input.unsplash_search').val('').trigger('input').trigger('change');
// Timeout before hiding unsplash so triggers do not overlap
setTimeout(function () {

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 13, 2018

Contributor

setTimeout is never the solution.

This comment has been minimized.

@kea14

kea14 Nov 13, 2018

Contributor

This is not the case anymore

@kea14

This comment has been minimized.

Contributor

kea14 commented Nov 13, 2018

Thank you @qsm-odoo
My branch has already been rebased but I did not push it.
Plus, I've already installed ESLint with odoo configuration (with this link that you sent me weeks ago)

Simplify your work, I doubt there should be an addition of 300+ lines for this task as it is about modifying existing features

I'll give it a try.

@qsm-odoo

This comment has been minimized.

Contributor

qsm-odoo commented Nov 13, 2018

The ESLint configuration has been updated in the docs, even more strict rules ;)

@qsm-odoo

This comment has been minimized.

Contributor

qsm-odoo commented Nov 15, 2018

@kea14 How is it going? Do not forget to push your work at least every day.

@kea14

This comment has been minimized.

Contributor

kea14 commented Nov 15, 2018

@qsm-odoo I'm a bit struggling on these :

  • Document upload is still broken
  • I've an error with Dialog.confirm

I pushed just now, maybe could you have a look when you have time ?
(I didn't push my work yesterday 'cause of wifi issues on my laptop)

@kea14

This comment has been minimized.

Contributor

kea14 commented Nov 15, 2018

@qsm-odoo Pushed with working Dialog.confirm

@qsm-odoo

This comment has been minimized.

Contributor

qsm-odoo commented Nov 15, 2018

@kea14 Ok, I found the problem for the document upload, I will make the fix in 12.0 or before myself. I will show you then.

@kea14

This comment has been minimized.

Contributor

kea14 commented Nov 15, 2018

@qsm-odoo Ok Thanks 😄 ! I'll read everything one more time and show it to PO. So, this task can be finished in early afternoon. Sounds good for you ?

@qsm-odoo

Some new comments and old comments remain 😉

  • No unsplash in web_editor
  • No 330 added lines for this task
@@ -186,18 +186,22 @@ var MediaWidget = Widget.extend({
var ImageWidget = MediaWidget.extend({
template: 'web_editor.dialog.image',
events: {
'click .o_upload_image_button': '_onUploadButtonClick',
'click .o_upload_image_button, .o_upload_document_button': '_onUploadButtonClick',

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 15, 2018

Contributor

Could you not share the logic like before ? Maybe rename both classes to 'o_upload_media_button' and share some of the xml template. Same for the URL button.

This comment has been minimized.

@kea14

kea14 Nov 15, 2018

Contributor

Ok. Little question: isn't it a problem about

trigger: ".o_select_media_dialog .o_upload_image_button",

in website_sale_tour_shop.js ?

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 15, 2018

Contributor

Well, when you rename something, you have to rename it everywhere. Including tours. Or you keep the old name. Your choice 😉

'input input.url': "_onSearchInput",
'click .existing-attachments [data-src]': '_onImageClick',
'dblclick .existing-attachments [data-src]': '_onImageDblClick',
'click .o_existing_attachment_remove': '_onRemoveClick',
'click .o_load_more_images': '_onLoadMore',
'input #imageurl': '_onImageURLInput',
'input #documenturl': '_onDocumentURLInput',

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 15, 2018

Contributor

Are not those two input event the same as the line input input.url above ? Should not the logic be there instead of two new functions ?

if (!self.multiImages) {
self.trigger_up('save_request');
}

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 15, 2018

Contributor

Was this decided ? No more leaving the media dialog on upload then ?

This comment has been minimized.

@kea14

kea14 Nov 15, 2018

Contributor

In the spec since the beginning, so yeah :)

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 15, 2018

Contributor

I do not see that in the specs, where are those specs ? 😄

This comment has been minimized.

@kea14

kea14 Nov 15, 2018

Contributor

"Images : The latests image uploaded or from URL appears first with a green-border" and sbu asked me several times.
Plus, 9 days ago I asked you this :

Do you know if it is conceivable in the _uploadImage function (in widgets.js - ImageWidget) to do something like that : Instead of closing modal when a new image is updated, we could get directly the new image next to the other ones without closing modal (and for instance having a green border around it). What do you think ?
(branch updated)

That was all about this.

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 15, 2018

Contributor

Yes, but I don't take decisions for functional stuff :) And I don't understand that spec like that but if SBU asked you directly, ok then :)

This comment has been minimized.

@kea14

kea14 Nov 15, 2018

Contributor

He did and also explained it to me. This isn't my decision 😃 I misspoke. I wanted your technical opinion on that one.

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 15, 2018

Contributor

My opinion is that FGI will want the opposite on the next saas version 😆 But no problem doing this now 😉

}
});
this.$el.submit();
// Emptying file inputs
this.$('#imagefile').val('');
this.$('#documentfile').val('');

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 15, 2018

Contributor

Is this useful ? (real question, I do not know)

This comment has been minimized.

@kea14

kea14 Nov 15, 2018

Contributor

Indeed, this seems not useful. I was trying things when was looking at document upload issue.. 😃

This comment has been minimized.

@kea14

kea14 Nov 15, 2018

Contributor

@qsm-odoo In fact it is useful. If you upload an image, and then if you add an image via URL this time, you'll get the previous image (not the one from the URL). Emptying file inputs is fixing this.

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 15, 2018

Contributor

Ok, but then, for a particular ImageWidget there is only one file input not two, and we don't have to care if we are using a document or an image at that moment. So something like

this.$('.o_file_input').val('');
} else {
$warning.toggleClass('d-none', success);
$success.toggleClass('d-none', !success);
this._nameButton('document', 'images');

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 15, 2018

Contributor

I think you did not understand the previous comment. Here you are using toggleClass in a context where the success variable is always false... so you could use addClass and removeClass directly. But I still think this should not be done this way. The whole function should end with those two toggleClass and using a boolean variable which is the result of all what has been checked above.

Also, this whole method is very close to _checkDocumentURL, I think what you want is a generic _checkURL handling both cases and using generic css classes to target the DOM elements.

} else {
this.active.goToPage(0);
this.active.search($(ev.currentTarget).val() || '');
}

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 15, 2018

Contributor

What is that function for ? I think the global search input has been removed ? In any case, the media dialog should not check what is inside his inner widget. Only call this.active.doStuff() like before.

This comment has been minimized.

@kea14

kea14 Nov 16, 2018

Contributor

I see. This function enables dropdown (only in images tab) in order to search in local images or unsplash images.

// Width search bar
.o_width_search_bar {
width: 20em;
}

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 15, 2018

Contributor

No need of CSS for this task. Review the bootstrap classes you are using.

<t t-call="web_editor.dialog.image.existing"/>
</div>
</t>
<t t-name="web_editor.dialog.document.submenu">

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 15, 2018

Contributor

Seems like that this is the same template as above, or nearly. At best, use generic class names and use the same template. At worst, make this template an extension of the first one (t-extend)

<div class="o_media_image_card card w-100">
<div class="card-body p-0">
<div class="card w-100">
<div class="card-body">

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 15, 2018

Contributor

I don't understand the point of using a card in this task.

This comment has been minimized.

@kea14

kea14 Nov 15, 2018

Contributor

In order to have a darken background in all tabs. Was asked in specs. Do you have a better idea ?

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 15, 2018

Contributor

Card backgrounds are supposed to be white. If it is not the case, this is because there is some CSS for that particular modal. Removing that card stuff and applying that CSS to the .tab-pane elements should be enough (all of this will remove line of code to achieve the specs 😉)

This comment has been minimized.

@kea14

kea14 Nov 20, 2018

Contributor

@qsm-odoo Btw, this was already the case in video tab and icons tab. Should it be also removed ?

@@ -142,16 +210,28 @@
</div>
</t>
<t t-name="web_editor.dialog.image.existing.content">
<div class="existing-attachments">
<div class="row mt16" t-as="row" t-foreach="rows">
<div class="existing-attachments container-fluid">

This comment has been minimized.

@qsm-odoo

qsm-odoo Nov 15, 2018

Contributor

No need of container inside a modal.

@qsm-odoo

This comment has been minimized.

Contributor

qsm-odoo commented Nov 15, 2018

@kea14 For information, here is the fix: 10183e8

@kea14 kea14 closed this Nov 20, 2018

@kea14 kea14 reopened this Nov 20, 2018

@robodoo robodoo added the CI 🤖 label Nov 20, 2018

m

@robodoo robodoo removed the CI 🤖 label Nov 20, 2018

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