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

[ADD] web_widget_slick: Add module #706

Merged
merged 4 commits into from
Sep 11, 2017

Conversation

hughesbm
Copy link

@hughesbm hughesbm commented Aug 23, 2017

Slick Carousel Widget

The web_widget_slick module provides a Slick Carousel widget for use in Odoo.

The web_widget_slick_example module provides an example implementation; views can be found in Settings / Slick

Usage

Default usage is on a One2many attachment field, as defined below:

class SlickExample(models.Model):
    _name = 'slick.example'
    _description = 'Slick Example Model'
    image_ids = fields.One2many(
        name='Images',
        comodel_name='ir.attachment',
        inverse_name='res_id',
    )

Assuming the above model, you would add a Slick Carousel on the image_ids column by using the following field definition in the model's form view:

<field name="image_ids" widget="one2many_slick_images" options="{}"/>

Options

The widget passes options directly through to Slick, so any setting available to Slick is available to the widget.

Additionally, Odoo-specific options fieldName and modelName are available; see readme for details.

@lasley lasley added this to the 10.0 milestone Aug 24, 2017
Copy link
Contributor

@lasley lasley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few inline comments. High level notes:

  • Only pull in the library files we need, so in this case just the slick JS (make sure the version number is in header)
    • I don't think we use the theme, but if you want to give it a shot go for it but use the LESS version. It's probably worth trying - it probably looks nice
  • Switch the icon to OCA

Odoo Slick Widget
=================

This module provides a Slick Carousel widget for use in Odoo.
Copy link
Contributor

@lasley lasley Aug 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"for use in the Odoo backend web interface"

@@ -0,0 +1,20 @@
/* Copyright 2017 LasLabs Inc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to LESS

I don't think you'll need to do anything other than the file extension and folder for the most part though.

Also this was like 2015 or something when I made this - double check the timeline for copyright.

* License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl.html). */

.slick-arrow{
background-color: #4c4c4c !important;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so I never really liked this part - the arrows look like crap if I remember right. We should maybe try to use fontawesome or glyphs, and maybe the built-in LESS colors

@hughesbm
Copy link
Author

hughesbm commented Aug 24, 2017

Styling has been updated to be more Odoo-like. Arrow buttons now use left/right font awesome chevrons (also used in paging buttons in Odoo) and LESS mixins of button classes to match other brand-colored buttons.

screen shot 2017-08-24 at 1 32 26 pm

@lasley
Copy link
Contributor

lasley commented Aug 24, 2017

Now that I'm thinking about it, let's go ahead and move the example module in too. It sucks, but I can't find any model in core that has a o2m attachments field that we could use 😦

@hughesbm
Copy link
Author

The example module has been added. 👍 ayyyyyy 👍

Example
-------

An example implementation can be found in the web_widget_slick_example module.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a note here about functionally testing this module using the example

@hughesbm hughesbm force-pushed the feature/10.0/SMD-263-mig-web_widget_slick branch 3 times, most recently from a900246 to 05cd4a3 Compare August 26, 2017 00:51
@hughesbm
Copy link
Author

PhantomJS is failing to find the field template on Travis, but works without any problems locally. The tests themselves all pass when run in the browser. Functional tests on runbot work as expected, too.

Any ideas on what could cause this?

Example Module
--------------

An example implementation, for instructional purposes as well as convenient
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does a functional reviewer find the menu for testing? Assume that a functional reviewer does not review code, so pointing them to code is no bueno

@pedrobaeza
Copy link
Member

Testing a bit this widget, I see that dragging over an image, there's a strange behavior that doesn't happen if you drag in the carousel outside an image. I see also that this control is readonly. Do you plan to add features for allowing to add or delete images?

@lasley
Copy link
Contributor

lasley commented Aug 30, 2017

I see also that this control is readonly. Do you plan to add features for allowing to add or delete images?

Not at this time, but that would be a good thing for roadmap.

We are, however, going to be implementing image editing with a module combining this with web_widget_darkroom from #595. The slick part will serve as an image browser with the darkroom part as the editor.

@hughesbm
Copy link
Author

@pedrobaeza I believe I found and dealt with the dragging behavior you mentioned; if it's not resolved for you and I just worked on a different dragging issue instead, please let me know! 😄

@lasley I updated the readme with more detailed instructions, and added a roadmap item regarding adding/deleting images from the carousel.

I also fixed some other issues related to lazy-loaded images not displaying properly while being sized responsively, especially when using the Slick grid settings.

@pedrobaeza
Copy link
Member

@hughesbm indeed, the dragging issue I mentioned is fixed with this commit, thanks!

@hughesbm
Copy link
Author

hughesbm commented Sep 1, 2017

Update: I was able to reproduce the PhantomJS failure to find the widget template locally, as well, so it's not a Travis issue. The template is loaded fine on subsequent test runs, but can't be found on the initial run.

@hughesbm
Copy link
Author

hughesbm commented Sep 6, 2017

The testing issue has been fixed! 🎉

Copy link
Contributor

@lasley lasley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hughesbm - comments inline

if (this.$slick) {
var $imgs = this.$el.find('img');
$imgs.each(function(idx, val){
self.$slick.slick('slickRemove', idx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this into a method slickRemove, delete the var self = this; above, and use proxy to send to destroy:

$imgs.each($.proxy(this.slickRemove, this));


this.$slick = $('<div class="slick-container"></div>');
this.$el.append(this.$slick);
self.loading.push.apply(self.get('value'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use this

self.loading.push.apply(self.get('value'));

_.each(self.get('value'), function(id){
var $img = $('<img class="img img-responsive"></img>');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this stuff into a slickRender then proxy to it just like recommended in slickRemove

In JS if you find yourself using the ol' this=self trick, you're probably nesting too much. I know I probably did this, so not your fault, but good to know regardless.

/* Copyright 2016-2017 LasLabs Inc.
* License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl.html). */

odoo.define('web_widget_slick.slick_widget', function(require){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can just call this web_widget_slick


<template>
<t t-name="FieldSlickImages">
<div t-att-class="'o_form_field '+widget.widget_class"></div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t-attf-class="o_form_field {{ widget.widget_class }}"

/* Copyright 2017 LasLabs Inc.
* License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl.html). */

odoo.define_section('web_widget_slick', ['web.core', 'web.form_common'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I typically suffix my tours with .tour, but that's stylistic so feel free to leave this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are just unit tests, not a tour. 😄

* License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl.html). */

odoo.define_section('web_widget_slick', ['web.core', 'web.form_common'],
function(test) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrmmmm this tabbing is bugging the crap out of me. I think it would be best to just leave the function declaration on the first line - it won't be that much longer than your other ones and we don't have a 79 char hard limit like we do in Python.

Copy link
Contributor

@lasley lasley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hughesbm

@hughesbm hughesbm force-pushed the feature/10.0/SMD-263-mig-web_widget_slick branch from d4e3c4f to 170ae9b Compare September 6, 2017 20:59
@hughesbm
Copy link
Author

hughesbm commented Sep 6, 2017

I just noticed there are display problems with multiple widgets per page, so this module still needs work.

@hughesbm
Copy link
Author

hughesbm commented Sep 9, 2017

The latest commit fixes display issues related to use of <group> and <sheet> tags. The widget should now display correctly and be sized appropriately.

<form string="Slick Example">
<header />
<sheet>
<field name="image_ids" widget="one2many_slick_images"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put this in a group to prove your fix works please

Copy link
Contributor

@lasley lasley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @hughesbm

Copy link

@felixvillafranca felixvillafranca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great¡

@pedrobaeza
Copy link
Member

@lasley squash at your taste before merging.

lasley and others added 4 commits September 11, 2017 07:40
* Create SlickJS widget field
* Remove template test
* Move web_widget_slick assets to backend only, update ReadMe, bump version
* Remove highlighting in ReadMe
* Add local slick files to use instead of CDN
* Override CSS to display widget correctly
* Adjust arrow button size/placement
* Bump version
* Rename __openerp__.py -> __manifest__.py
* Rename widget_slick.js -> web_widget_slick.js
* Update copyright, license (AGPL -> LGPL)
* Update readme
* Correct eslint errors
* Change module name in various places for consistency
* Improve styling of widget arrows, dots
* Change css -> less
* Remove unneeded slick files
* Copyright 2017 -> 2016-2017
* Add OCA to authors
* Use OCA icon
* Fix readme
* Clean up assets
* Fix file permissions
* Update readme with reference to example module
* Fix formatting error, incorrect link
* Add javascript tests
* Add note to readme about functional testing with example module
* Fix/cleanup javascript
  * Fix destroy_content() method
  * Move slide navigation out of slide addition loop
  * Remove unused variables
  * Remove unneeded DOM append
* Reorganize files/directories
* Adjust template tags (templates -> template)
* Add slick-field class to field template instead of using jQuery
* Misc cleanup
* Adjust breakpoint settings to show fewer images by default
* Enable adaptiveHeight by default
* Add .img and .img-responsive classes to images
* Fix dragging issues by preventing default mousedown and
  touchstart event behavior
* Set swipeToSlide default to true
* Change how slick slides are populated to allow grid mode
* Fix issue causing carousel images to display improperly in some
  situations
* Add better functional testing instructions to readme
* Add roadmap to readme
* Make minor styling changes
* Fix issue with template loading w/ PhantomJS
* Clean up template, use css class provided by widget
* Remove unneeded dependency from tests
* Break up render_value method
* Break up destroy_content method
* Add unslicking to destroy_content, add test
* Clean up qweb template formatting
* Fix indentation
* Change widget name
* Add Slick copyright information
* Add padding left/right, move arrows in to avoid clipping when
  widget not in a sheet tag
* Apply dot and arrow styles only when needed
* Add _resizeCarousel() and related methods to ensure accurate
  carousel sizing in various views
  * Resize carousel on core.bus resize
  * Account for differences in group layouts and labels,
    sheet/no-sheet layouts
* Adjust, clean up less
* Clean up js
* Create SlickJS widget field in example module for usage instructions
* Add security to example slick widget model
* Bump version
* Rename __openerp__.py -> __manifest__.py
* Replace openerp import, tags with odoo
* Remove data tags
* Replace LasLabs icon w/ OCA icon
* Update readme
* Update license (AGPL -> LGPL)
* Update copyright
@lasley lasley force-pushed the feature/10.0/SMD-263-mig-web_widget_slick branch from 8a2b90b to 63bfc0f Compare September 11, 2017 14:45
@lasley
Copy link
Contributor

lasley commented Sep 11, 2017

I ended up reordering some of the commits as well so that we could have a clean 4 commits - one for each module creation by me, then another for each module upgrade by Brent.

I'll merge once we get a CI 🍏

@lasley lasley merged commit ad466e2 into OCA:10.0 Sep 11, 2017
@lasley lasley deleted the feature/10.0/SMD-263-mig-web_widget_slick branch September 11, 2017 15:22
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.

4 participants