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 an option to return inline style even if we want to avoid it at the beginning #5933

Merged

Conversation

Dobby85
Copy link
Contributor

@Dobby85 Dobby85 commented Jun 8, 2024

Hi 👋

Here is the context why I'm creating this PR. I saw that the config parameter avoidInlineStyle is now deprecated because of responsive and state management. But in some case we could still need inline style.

For example, if you get a dynamic list with multiple elements and each element has a specific color that you want to use as text color. You can only do it with inline style. But if here we set inline style, this is because you want it and you know you will cannot handle responsive and state on the text color.

avoidInlineStyle = true

If I understand correctly, avoidInlineStyle is set to true by default to handle responsive and state management. We can still add inline style with setStyle('color: red;', { inline: true }) but when we get the final HTML with toHTML(), the inline style is remove and style is lost.

Screenshot 2024-06-08 at 09 32 25

avoidInlineStyle = false

When we put avoidInlineStyle to false, and we do setStyle('color: red;', { inline: true }), the inline style is set correctly but at the CSS generation (editor.getCss()), the same style is also added in the CSS code. What we don't want because we explicitly specify { inline: true } when setting the style.

Screenshot 2024-06-08 at 09 29 59

What do I change?

So I just add one export parameter in ToHTMLOptions named keepInlineStyle to allow inline style when it is explicitly wanted.

By doing that, I saw that the test suite Component was running an editor with avoidInlineStyle set to false, which is not the default way of working now if I understand. So I set it to true in the test suite and fix 3 tests that work a little but differently.

To conclude

// Set style on #id or .class
cmp.setStyle({ color: 'red' })
// OR
cmp.setAttributes({ style: 'color: red' })

// Style has not been set as an attributes but as a CSS rule
console.log(cmp.getAttributes()) // {}
console.log(cmp.getStyle()) // { color: 'red' }


// Set style on #id or .class
cmp.setStyle({ color: 'red' },  { inline: true })
// OR
cmp.setAttributes({ style: 'color: red' }, { inline: true })

// Style has not been set as an attributes but as a CSS rule
console.log(cmp.getAttributes()) // { style: 'color: red;' }
console.log(cmp.getStyle()) // {}
console.log(cmp.getStyle({ inline: true })) // { color: 'red' }

I hope my PR is clear and you will understand why I did this.

PS: Something totally different, I add the keymaps config key in the editor config type to avoid typescript check issue when using it.

Do not hesitate if you have any reviews or comments!

Copy link
Member

@artf artf left a comment

Choose a reason for hiding this comment

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

Makes all sense, I'd like to stay away from inline styles in general but this is actually a nice fix for the inline option.

Thank you also for the effort of making a good PR (context of the issue, comments in the right place, tests, etc.) 👏

@@ -5,7 +5,7 @@ interface NOOP {}

export type Debounced = Function & { cancel(): void };

export type SetOptions = Backbone.ModelSetOptions & { avoidStore?: boolean };
export type SetOptions = Backbone.ModelSetOptions & { avoidStore?: boolean; inline?: boolean };
Copy link
Member

Choose a reason for hiding this comment

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

I think this one could be moved to UpdateStyleOptions type (I'll do it)

/**
* Configurations for keymaps
*/
keymaps?: KeymapsConfig;
Copy link
Member

Choose a reason for hiding this comment

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

🙇‍♂️

Comment on lines +301 to +311
test('set style on id and inline style', () => {
obj.setStyle({ color: 'red' }); // Should be set on id
obj.setStyle({ display: 'flex' }, { inline: true }); // Should be set as inline

expect(obj.getStyle()).toEqual({
color: 'red',
});
expect(obj.getStyle({ inline: true })).toEqual({
display: 'flex',
});
});
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@artf artf merged commit 509e410 into GrapesJS:dev Jun 16, 2024
2 checks passed
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.

2 participants