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 snippet options owl bso #3299

Draft
wants to merge 112 commits into
base: master-snippet-options-owl-ard
Choose a base branch
from

Conversation

bso-odoo
Copy link

PR created to ease discussion during the conversion of snippet options to Owl

Local temporary work based on ARD's branch

@robodoo
Copy link

robodoo commented Jun 21, 2024

This PR targets the un-managed branch odoo-dev/odoo:master-snippet-options-owl-ard, it needs to be retargeted before it can be merged.

@bso-odoo bso-odoo force-pushed the master-snippet-options-owl-bso branch 5 times, most recently from 6e6892f to 9153992 Compare June 24, 2024 08:25
Copy link

@detrouxdev detrouxdev left a comment

Choose a reason for hiding this comment

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

Thanks for your work! I just have a couple questions on the few commits I reviewed, and maybe it can also guide the next commits?

Mostly surface things, the rest seems to be good 👍🏻

addons/web_editor/static/src/js/editor/snippets.options.js Outdated Show resolved Hide resolved
addons/website/static/src/js/editor/snippets.editor.js Outdated Show resolved Hide resolved
Comment on lines 196 to 255
const themeFontsNb = nbFonts - (this.googleLocalFonts.length + this.googleFonts.length);
const googleLocalFontsOffset = nbFonts - this.googleLocalFonts.length;
for (let fontNb = 0; fontNb < nbFonts; fontNb++) {
const realFontNb = fontNb + 1;
const fontKey = weUtils.getCSSVariableValue(`font-number-${realFontNb}`, style);
this.allFonts.push(fontKey);
let fontName = fontKey.slice(1, -1); // Unquote
let fontFamily = fontName;
const isSystemFonts = fontName === "SYSTEM_FONTS";
if (isSystemFonts) {
fontName = _t("System Fonts");

Choose a reason for hiding this comment

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

This was copied from WeFontFamilyPicker widget, but the code was not removed, shouldn't it be?

Copy link
Author

Choose a reason for hiding this comment

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

I actually forgot to get rid of the component's start method which did build the buttons "manually" - because this is now done in the template using the data structure built in the FontFamilyUserValue.

addons/web_editor/static/src/js/editor/snippets.options.js Outdated Show resolved Hide resolved
Comment on lines 894 to 906
if (htmlContent && htmlContent !== this.state.textContent) {
const markupContent = markup(htmlContent);
// Avoid converting the markup back to a string.
markupContent.trim = () => markupContent;
this.state.textContent = markupContent;

Choose a reason for hiding this comment

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

Actually, why does this happen? After thinking a bit about it, we might want to make sure the markup is not trimmed where it currently is, in case we need to perform other string operations, we would have to void the methods here as well. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

I tried to do that, but I found no way to "safely" determine if something is a markup.
IMO (but yours might differ), checking .constructor.name === "Markup" looks like an even worse hack.

Or we could go for a distinct state field markupContent ? And handle it differently on the other side ?

@bso-odoo bso-odoo force-pushed the master-snippet-options-owl-bso branch 4 times, most recently from 0c65909 to d11fc02 Compare June 25, 2024 07:38
@bso-odoo bso-odoo force-pushed the master-snippet-options-owl-bso branch from d11fc02 to a44ba52 Compare June 25, 2024 09:43
Comment on lines 329 to 552
this.state.option.instance._onGoogleFontsCustoRequest({
// TODO: @owl-options should we allow something else that "variable" ? (most likely not)
values: {[this.props.variable]: `'${font}'`},

Choose a reason for hiding this comment

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

Not a huge fan of this, but OK for now. Let's just keep in mind that we should maybe have a way for widgets to request something from the option.

@detrouxdev detrouxdev force-pushed the master-snippet-options-owl-ard branch from fb98eb8 to 50c6b00 Compare June 25, 2024 14:35
@bso-odoo bso-odoo force-pushed the master-snippet-options-owl-bso branch 15 times, most recently from b384892 to dd127ca Compare July 1, 2024 15:08
@bso-odoo bso-odoo force-pushed the master-snippet-options-owl-bso branch from 06f3c99 to 3de47f4 Compare August 6, 2024 14:08
@detrouxdev detrouxdev force-pushed the master-snippet-options-owl-ard branch 9 times, most recently from 633ee8d to 1240c73 Compare August 20, 2024 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants