Skip to content

[11.0][MIG] website snippet anchor#420

Merged
yajo merged 5 commits intoOCA:11.0from
tarteo:11.0-mig-website_snippet_anchor
Feb 26, 2018
Merged

[11.0][MIG] website snippet anchor#420
yajo merged 5 commits intoOCA:11.0from
tarteo:11.0-mig-website_snippet_anchor

Conversation

@tarteo
Copy link
Member

@tarteo tarteo commented Feb 7, 2018

No description provided.

yajo and others added 4 commits February 7, 2018 09:21
Remove unused keys from manifest.

Enable the posibility to link only to an anchor.

Update all dependencies from an anchor too.

This makes carousels work again when changing their anchor, and makes Odoo
save changed anchors even when no visible changes are made on the elements.
@tarteo tarteo changed the title [11.0][MIG]website snippet anchor [11.0][MIG] website snippet anchor Feb 7, 2018
@tarteo tarteo mentioned this pull request Feb 7, 2018
38 tasks
@pedrobaeza pedrobaeza added this to the 11.0 milestone Feb 7, 2018
Copy link

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

just small issues. Code LGTM.


.. image:: https://odoo-community.org/website/image/ir.attachment/5784_f2813bd/datas
:alt: Try me on Runbot
:target: https://runbot.odoo-community.org/runbot/186/10.0
Copy link

Choose a reason for hiding this comment

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

/11.0

"summary": "Allow to reach a concrete section in the page",
"version": "11.0.1.0.0",
"category": "Website",
"website": "https://www.tecnativa.com",
Copy link

Choose a reason for hiding this comment

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

@tarteo
Copy link
Member Author

tarteo commented Feb 9, 2018

@simahawk Thanks, I've applied your comments.

@rgarnau
Copy link

rgarnau commented Feb 15, 2018

LGTM 👍

Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Functionally tested and it works.
Some migration remarks though.

var url = this.data.url || "",
url_parts = url.split("#", 2);
if (url_parts.length > 1) {
console.log(url_parts);
Copy link
Member

Choose a reason for hiding this comment

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

🔥

<li data-anchor_ask=""
data-only="click"
title="Use anchors to go directly to a section of your page, appending #anchor to the URL">
<li title="Use anchors to go directly to a section of your page, appending #anchor to the URL">
Copy link
Member

Choose a reason for hiding this comment

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

I prefer the previous way, since it just follows the standard for snippet options. Please restore it if possible.

Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

I think you're changing things because of taste before understanding how the snippet options api works... 🤔

<xpath expr="//div[@id='snippet_options']" position="inside">
<div data-js="anchor"
data-selector="section, .carousel">
<li data-anchor_ask=""
Copy link
Member

Choose a reason for hiding this comment

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

There was a data-only="click" here before. Leave it as it was if you want to avoid hover events.

Copy link
Member Author

@tarteo tarteo Feb 21, 2018

Choose a reason for hiding this comment

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

No, this attribute is not implemented anymore. I'll use the new data-no-preview="true" instead.

Copy link
Member

Choose a reason for hiding this comment

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

Oh great, I didn't know that 😉


anchor_ask: function(type) {
if(type !== false) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Since the good result is a deferred, it makes more sense to return $.Deferred().reject() when failing. Leave as it was please.

options.registry.anchor = options.Class.extend({

anchor_ask: function(type) {
if(type !== false) {
Copy link
Member

Choose a reason for hiding this comment

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

type is the event type. You want to react only in click event, so leave it as it was: if (type !== "event")

Copy link
Member Author

Choose a reason for hiding this comment

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

No, when on click type is in 11.0 false. I'll remove it when I apply the attribute data-no-preview

@tarteo
Copy link
Member Author

tarteo commented Feb 21, 2018

@yajo I know how the options for snippets work... Do not try to assume things next time.

@yajo
Copy link
Member

yajo commented Feb 21, 2018

Yikes, then it was me that didn't know the api changes on v11! Thanks, didn't want to bother you 😇

Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Perfect. Please squash migration commits to merge 😊

[FIX] Fixed minor template / migration issues

[IMP] Use standard Odoo options mechanic

[REM] unused start function

[IMP] Use data-no-preview

[FIX] new line after list (README)
@yajo yajo merged commit b2079c1 into OCA:11.0 Feb 26, 2018
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.

6 participants