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

[MERGE][REF] website_slides: allow / improve channel and slide creation #30991

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@jem-odoo
Copy link
Contributor

jem-odoo commented Feb 11, 2019

This merge provides new modal to create slide channel from the 'New' website
menu, like blogpost or forum. It also improves the upload slide widget from
channel homepage.

This merge also brings new slide type 'webpage'. This allow user to create
completely customized content as a slide.

This merge is linked to Task-1938643. More generally it is linked to the
eLearning feature [1]. For more details see subcommits.

[1] see task ID 1902304 (main eLearning task) PR #29876

@robodoo robodoo added the seen 🙂 label Feb 11, 2019

@jem-odoo jem-odoo force-pushed the odoo-dev:master-slides-jem branch 2 times, most recently from 90e528a to f1b2038 Feb 11, 2019

@C3POdoo C3POdoo added the RD label Feb 11, 2019

@jem-odoo jem-odoo force-pushed the odoo-dev:master-slides-jem branch 3 times, most recently from 843f4c6 to e5e92ef Feb 11, 2019

@jem-odoo jem-odoo force-pushed the odoo-dev:master-slides-jem branch from e5e92ef to 5e32fc5 Feb 11, 2019

@robodoo robodoo added the CI 🤖 label Feb 11, 2019

@jem-odoo jem-odoo force-pushed the odoo-dev:master-slides-jem branch from 5e32fc5 to e7b25f1 Feb 11, 2019

@robodoo robodoo removed the CI 🤖 label Feb 11, 2019

@jem-odoo jem-odoo force-pushed the odoo-dev:master-slides-jem branch 2 times, most recently from 90bb8ba to f2747cc Feb 11, 2019

@tde-banana-odoo
Copy link
Contributor

tde-banana-odoo left a comment

First technical review, see comments. Thanks for your work !

I have a new spec for you: as channel tags are now available in master you can add them in channel creation widget :) .

Show resolved Hide resolved addons/website_slides/controllers/main.py Outdated
Show resolved Hide resolved addons/website_slides/models/slide_channel.py Outdated
}),
xmlDependencies: WebsiteNewMenu.prototype.xmlDependencies.concat(
['/website_slides/static/src/xml/website_slides.xml']
['/website_slides/static/src/xml/website_slide_channel.xml']

This comment has been minimized.

@tde-banana-odoo

tde-banana-odoo Feb 11, 2019

Contributor

slide_channel_upload.xml ?

This comment has been minimized.

@jem-odoo

jem-odoo Feb 11, 2019

Author Contributor

it contains the template of the modal to create new channel. I would keep it like this.

This comment has been minimized.

@tde-banana-odoo

tde-banana-odoo Feb 12, 2019

Contributor

I think it should be either slide_channel_something, either website_slides_channel (wiht an S), because someone merged this module with an 'S' at slides ... sad but true !

Show resolved Hide resolved addons/website_slides/controllers/main.py Outdated
Show resolved Hide resolved addons/website_slides/models/slide_slide.py Outdated
Show resolved Hide resolved addons/website_slides/security/ir.model.access.csv Outdated
Show resolved Hide resolved addons/website_slides/models/slide_channel.py Outdated
Show resolved Hide resolved addons/website_slides/models/slide_slide.py Outdated
Show resolved Hide resolved addons/website_slides/models/slide_slide.py
},
_fetchUrlPreview: function (url) {
return this._rpc({
route: '/slides/dialog_preview/',

This comment has been minimized.

@tde-banana-odoo

tde-banana-odoo Feb 11, 2019

Contributor

While we are at it, couldn't we rename that route ? dialog_preview is quite moche. Not important anyway, just passing by.

This comment has been minimized.

@jem-odoo

jem-odoo Feb 11, 2019

Author Contributor

yes it is moche. Does slides/previwe_values/ is better? I have no really good idea ....

@robodoo robodoo added the CI 🤖 label Feb 11, 2019

@jem-odoo jem-odoo force-pushed the odoo-dev:master-slides-jem branch from f2747cc to 0f7cb8c Feb 11, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 11, 2019

@jem-odoo jem-odoo force-pushed the odoo-dev:master-slides-jem branch from 6e33646 to 0d6ad68 Feb 12, 2019

@awa-odoo
Copy link
Contributor

awa-odoo left a comment

Mostly camel case and other small changes for a better JS world.
Also some weird stuff worth double checking.

Show resolved Hide resolved addons/website_slides/controllers/main.py Outdated
Show resolved Hide resolved addons/website_slides/static/src/js/slides_upload.js Outdated
/**
* @override
* @param {Object} el
* @param {Object} parent
* @param {Object} data holding channelId and optionally upload and publish control parameters

This comment has been minimized.

@awa-odoo

awa-odoo Feb 12, 2019

Contributor

"data" -> "options"

Show resolved Hide resolved addons/website_slides/static/src/js/slides_upload.js Outdated
Show resolved Hide resolved addons/website_slides/static/src/js/slides_upload.js Outdated
Show resolved Hide resolved addons/website_slides/static/src/xml/website_slide_upload.xml Outdated
Show resolved Hide resolved addons/website_slides/static/src/xml/website_slide_upload.xml Outdated
Show resolved Hide resolved addons/website_slides/views/slide_slide_views.xml Outdated
@@ -182,6 +182,12 @@
<span class="badge badge-pill" t-esc="header_object.nbr_infographics"/> Infographics
</a>
</li>
<li t-if="header_object.nbr_webpages" class="nav-item">
<a t-attf-href="/slides/#{slug(channel)}#{category and '/category/' + slug(category) or ''}/webpage"
t-attf-class="nav-link#{slide_type == 'webpage' and ' active' or ''}">

This comment has been minimized.

@awa-odoo

awa-odoo Feb 12, 2019

Contributor

Maybe it's just because I'm not used to that notation it but I would prefer ' active' if slide_type == 'webpage' else ''

(Same thing for the line above)

This comment has been minimized.

@jem-odoo

jem-odoo Feb 12, 2019

Author Contributor

Just copy/paste. Maybe tde will rewrite all this templates in its branch with the new homepage.

<div role="tabpanel" class="tab-pane fade oe_slides_transcript" id="external-links" t-if="slide.link_ids">
<ul>
<t t-foreach="slide.link_ids" t-as="link">
<li><a t-att-href="link.link" target="blank"><t t-esc="link.name"/></a></li>

This comment has been minimized.

@awa-odoo

awa-odoo Feb 12, 2019

Contributor

"blank" -> "_blank" ?

@jem-odoo jem-odoo force-pushed the odoo-dev:master-slides-jem branch 3 times, most recently from 0cdfd75 to a678608 Feb 12, 2019

@tde-banana-odoo
Copy link
Contributor

tde-banana-odoo left a comment

Tech review: LGTM, some small changes.

Show resolved Hide resolved addons/website_slides/static/src/js/slides_upload.js Outdated
var SlideUploadDialog = Dialog.extend({
template: 'website.slide.upload.modal',
events: _.extend({}, Dialog.prototype.events, {
'click .o_w_slide_select_type': '_onClickSlideTypeIcon',

This comment has been minimized.

@tde-banana-odoo

tde-banana-odoo Feb 12, 2019

Contributor

Technically selectors should be o_wslides_YYY ^^

}),
xmlDependencies: WebsiteNewMenu.prototype.xmlDependencies.concat(
['/website_slides/static/src/xml/website_slides.xml']
['/website_slides/static/src/xml/website_slide_channel.xml']

This comment has been minimized.

@tde-banana-odoo

tde-banana-odoo Feb 12, 2019

Contributor

I think it should be either slide_channel_something, either website_slides_channel (wiht an S), because someone merged this module with an 'S' at slides ... sad but true !

Show resolved Hide resolved addons/website_slides/static/src/xml/website_slide_upload.xml Outdated

@robodoo robodoo added the CI 🤖 label Feb 12, 2019

@jem-odoo jem-odoo force-pushed the odoo-dev:master-slides-jem branch 2 times, most recently from 132372b to c49bfb7 Feb 12, 2019

[IMP] website_slides: create channel in frontend
This commit provides the abilty to create channel from the website
instead of creating a slide. To do so, the user still have the "upload"
button on the channel home page.
The new modal allow user to set title, description, type and channel
tags. We prevent channel tag and group tag creation on the fly, as it
can impact the UI (coming in a future commits).

Task-1938643

qmo-odoo added some commits Feb 8, 2019

[IMP] website_slides: add external links on slide
A slide can now have some external link. For instance, links to
the origin of the document (sources for scientifics papers, ...).
A new model is added to allow the user to provide several link
per slide. Those are display below the channel on website page.

Task-1938643
[IMP] website_slides: webpage slide type
The idea is a slide can be a HTML page completely customized by
user. So, you can now easily create custom content and publish
it on your website, like a normal slide.

Task-1938643
[IMP] website_slides: remove unique slide name constraint within channel
Having a unique constraint for a slide name in a channel look strange.
It force an RPC call to check this when uploading a slide, making the
process a little bit slow.
Morevover, name is a translatable field on slide.slide model, so the
constraint can not be fully respected.

To simplify the model, we decided to remove this.

Task-1938643
[IMP] website_slides: new modal to upload a document
With new slide type, it was required to change the
upload modaL. Now, the use select first the type
of document he wants to upload, then the correct
form appears. When filled, user can sumbit it to
be redirect to the slide he just create.

Task-1938643

@jem-odoo jem-odoo force-pushed the odoo-dev:master-slides-jem branch from c49bfb7 to 2d25869 Feb 12, 2019

@tde-banana-odoo tde-banana-odoo changed the title Master slides jem [MERGE][REF] website_slides: allow / improve channel and slide creation Feb 12, 2019

@tde-banana-odoo

This comment has been minimized.

Copy link
Contributor

tde-banana-odoo commented Feb 12, 2019

@robodoo r+ rebase-merge

@robodoo robodoo added the r+ 👌 label Feb 12, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 12, 2019

Merge method set to rebase and merge, using the PR as merge commit message

robodoo added a commit that referenced this pull request Feb 12, 2019

[MERGE][REF] website_slides: allow / improve channel and slide creation
This merge provides new modal to create slide channel from the 'New' website
menu, like blogpost or forum. It also improves the upload slide widget from
channel homepage.

This merge also brings new slide type 'webpage'. This allow user to create
completely customized content as a slide.

This merge is linked to Task-1938643. More generally it is linked to the
eLearning feature [1]. For more details see subcommits.

[1] see task ID 1902304 (main eLearning task) PR #29876

closes #30991
@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 12, 2019

Merged, thanks!

@robodoo robodoo closed this Feb 12, 2019

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