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 content editor data attributes based on name/id and css_classes presenter method #1761

Merged

Conversation

mickenorlen
Copy link
Contributor

@mickenorlen mickenorlen commented Mar 24, 2020

What is this pull request for?

To enable more unique targetable classes for styling and manipulating contents in backend forms.

Notable changes (remove if none)

Enable users to specify classes in content definition:

- name: element
  contents:
    - name: content_with_classes
      type: EssenceText
      classes: content_classes

Added classes method to content.rb and injected it to all essences + generator template wrapper classes.

Added test for:

  • classes in content_spec.rb
  • checking that classes are correctly rendered in page_editing_feature_spec.rb

Checklist

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have added tests to cover this change

Copy link
Member

@tvdeyen tvdeyen 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 the contribution. I like the idea, but think we can achieve the same with either

a. Using the content name as css class in the editor. Maybe also add the element name to the element editor? This should be a good enough selector, right?
b. If we still want to support adding additional classes to the editor. Why not use the settings hash we already have for these things? I do not want to add additional attributes to the Model. Also we already have a presenter class, that would be a great home for this method.

Thanks again for the contribution.

app/models/alchemy/content.rb Outdated Show resolved Hide resolved
spec/dummy/config/alchemy/elements.yml Outdated Show resolved Hide resolved
@mickenorlen mickenorlen force-pushed the user-defined-content-classes branch from 6d9dd8c to 91fb6df Compare March 25, 2020 10:19
@mickenorlen
Copy link
Contributor Author

a) Added default element name and content name classes to editor.
b) Added custom editor_css_classes to contents as I find it convenient to use helper classes for these guys that can then be toggled through js.

Opinion/question: If i read here.
https://guides.alchemy-cms.com/essences.html#overview

It seems that global configuration should go in the top level conf and "individual" settings go into settings hash. So i think this one could still be top level?

@mickenorlen mickenorlen requested a review from tvdeyen March 25, 2020 10:28
@tvdeyen
Copy link
Member

tvdeyen commented Mar 25, 2020

Opinion/question: If i read here.
https://guides.alchemy-cms.com/essences.html#overview

It seems that global configuration should go in the top level conf and "individual" settings go into settings hash. So i think this one could still be top level?

I think I cannot follow? Content and Essence have a one-to-one relationship there is no "global" and "individual" config for either of them, they are the same. The content is being of an essence_type. The essence record is the individual instance of each content defined, but the same is true for the content instance in each element instance.

Copy link
Member

@tvdeyen tvdeyen 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 the changes.

But thinking about this again I think we should add the element.name to a data-element-name attribute and the content.name to a data-content-name attribute.

With that you can do crazy things like:

[data-element-name="blog_settings"] [data-content-name="author_name"] {
  font-weight: bold;
  color: red;
}

or

document.querySelectorAll('[data-element-name="video_header"] [data-content-name="embed_type"]').addEventListener('change', (evt) => {
  evt.target.value
  ...
})

Should be fixing you need (flexibility in the content editors) and ours (less maintenance burden)

WDYT?

app/decorators/alchemy/content_editor.rb Outdated Show resolved Hide resolved
app/decorators/alchemy/content_editor.rb Outdated Show resolved Hide resolved
app/decorators/alchemy/content_editor.rb Outdated Show resolved Hide resolved
app/decorators/alchemy/element_editor.rb Outdated Show resolved Hide resolved
app/views/alchemy/essences/_essence_select_editor.html.erb Outdated Show resolved Hide resolved
app/views/alchemy/essences/_essence_text_editor.html.erb Outdated Show resolved Hide resolved
spec/dummy/config/alchemy/elements.yml Outdated Show resolved Hide resolved
@mickenorlen
Copy link
Contributor Author

From: https://guides.alchemy-cms.com/essences.html#overview

Global content settings

When defining contents, you need to provide a name and essence type. You can set hints and default values as well.

Individual essence settings

Each essence type can have its own type of settings.

To configure these settings you have to pass them into its settings key in the elements.yml.

From that it seems that editor_css_classes which would be available for all contents shouldn't go under the settings hash.

@tvdeyen
Copy link
Member

tvdeyen commented Mar 25, 2020

From: https://guides.alchemy-cms.com/essences.html#overview

Global content settings

When defining contents, you need to provide a name and essence type. You can set hints and default values as well.

Individual essence settings

Each essence type can have its own type of settings.
To configure these settings you have to pass them into its settings key in the elements.yml.

From that it seems that editor_css_classes which would be available for all contents shouldn't go under the settings hash.

This might be poorly worded, but my comment is still valid. But since we want to implement this differently this discussion is not necessary any more ;)

@mickenorlen mickenorlen force-pushed the user-defined-content-classes branch from 91fb6df to a92b04b Compare March 28, 2020 10:06
@mickenorlen mickenorlen changed the title Add user defined content classes Add content editor data attributes based on name/id and css_classes presenter method Mar 28, 2020
@mickenorlen
Copy link
Contributor Author

Thanks for good comments. I think i fixed it now. I did however use separate data-content-id and data-content-name instead of a nested object.

Because i saw that there were already a few data-content-id attributes in use in the code eg here
https://github.com/mickenorlen/alchemy_cms/blob/user-defined-content-classes/app/assets/javascripts/alchemy/alchemy.link_dialog.js.coffee/#L155
And also it felt a bit cryptic to target data attribute objects in general.

[data-content*="name\":\"date"] {
	background: blue;
}

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Very nice, just one little thing. Thanks for the contribution 🙏

app/decorators/alchemy/content_editor.rb Outdated Show resolved Hide resolved
@mickenorlen
Copy link
Contributor Author

Np i'll clean that up.

But I think i want to create a ContentEditor#dom_data_attributes also so it's easier to inject data-attributes when needed without changing the templates.

data: {content_id: essence_text_editor.id, content_name: essence_text_editor.name}

# would become
data: essence_text_editor.dom_data_attributes

Ok?

@mickenorlen mickenorlen force-pushed the user-defined-content-classes branch from a92b04b to b0cc24f Compare April 2, 2020 12:20
@mickenorlen mickenorlen requested a review from tvdeyen April 2, 2020 12:22
@tvdeyen
Copy link
Member

tvdeyen commented Apr 2, 2020

Np i'll clean that up.

But I think i want to create a ContentEditor#dom_data_attributes also so it's easier to inject data-attributes when needed without changing the templates.

data: {content_id: essence_text_editor.id, content_name: essence_text_editor.name}

# would become
data: essence_text_editor.dom_data_attributes

Ok?

Yes

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Very nice. One last change and we are ready to go. Thank you for working on this.

app/decorators/alchemy/content_editor.rb Outdated Show resolved Hide resolved
@tvdeyen tvdeyen added this to the 5.0 milestone Apr 2, 2020
@mickenorlen mickenorlen force-pushed the user-defined-content-classes branch from b0cc24f to 513d273 Compare April 3, 2020 12:46
Added data_content_name, data_content_id for all
@mickenorlen mickenorlen force-pushed the user-defined-content-classes branch from 513d273 to 5c2e70b Compare April 3, 2020 12:49
@mickenorlen mickenorlen requested a review from tvdeyen April 3, 2020 12:52
@tvdeyen tvdeyen merged commit a43a746 into AlchemyCMS:master Apr 3, 2020
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