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

Remove local options from essence editors #1638

Merged
merged 7 commits into from
Oct 21, 2019

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented Oct 16, 2019

What is this pull request for?

Why? Passing around options hashes from the element editor views into the essence editor views and even in the params is error prone and not necessary in most of the time. We already have the settings key on the contents definition in the elements.yml.

Notable changes

This removes the local options and html_options hashes passed to the essence editor partials. Stop passing local options from all element editor partials into essence editors.

Please set all options you have passed into the essence editor on the content definition itself.

Example

<%= element_editor_for(element) do |el| %>
  <%= el.edit :image, size: '2000x800', crop: true %>
<% end %>

will become

- name: hero
  contents:
    - name: image
      type: EssencePicture
      settings:
        size: 2000x800
        crop: true

Q: But what about dynamic options?

Sometimes you want to pass dynamic instead of static options into the essence editor. Like for a selection of pages you want to let chose an editor from.

A: Create an essence for this

Using an essence for multiple purposes is often not the best way of implementing those use cases. Imagine a event select for instance.

Instead of storing the Event#id in a string column of the EssenceText or EssenceSelect you should add your own custom essence (say EssenceEvent) and add a event_id foreign key and set the event as ingredient_column.

Another good example is the Alchemy Solidus extension. It provides EssenceSpreeProduct and EssenceSpreeTaxon essences to work with.

This is a much more reliable approach of using dynamic values in your Alchemy instance.

B: Use EssencePage to reference pages

If you want to reference a page you can use the newly introduced EssencePage.

@tvdeyen tvdeyen added this to the 5.0 milestone Oct 16, 2019
@tvdeyen tvdeyen force-pushed the no-local-options-in-editors branch 2 times, most recently from 014c7e2 to f575b22 Compare October 16, 2019 09:43
Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

Yay for the simplification, this is great I think. However, I believe a deprecation warning makes sense if the functionality is still there but to be removed in the future. As we're removing the functionality in this PR, I think it's more appropriate to raise instead.

app/helpers/alchemy/admin/essences_helper.rb Outdated Show resolved Hide resolved
app/helpers/alchemy/admin/essences_helper.rb Outdated Show resolved Hide resolved
@tvdeyen
Copy link
Member Author

tvdeyen commented Oct 16, 2019

@mamhoff #1642 adds deprecations

@mtomov
Copy link
Contributor

mtomov commented Oct 17, 2019

Yeah, absolutely for - have never used that.

@tvdeyen tvdeyen force-pushed the no-local-options-in-editors branch 4 times, most recently from 2214120 to 477287f Compare October 18, 2019 10:59
@tvdeyen tvdeyen changed the base branch from master to five October 21, 2019 18:55
And the default value set correctly on the newly build
essence object.
Read the crop and size settings from one canonical source the content
settings defined in the elements.yml

This reduces complexity and prevents hard to track down errors.
Rely only on the content settings for sizing and cropping
settings.
All settings a content might have are set on the contents
definition settings key.
This was only done because we supported to have local options in
the element editor and essence editor partials. We do not support
this anymore and expect to use static content.settings instead.
Passing options as locals to essence editors is not possible anymore.

Static values should be set on the content settings on the contents
defintion in elements.yml instead. For dynamic values please use a
custom essence class.
@tvdeyen tvdeyen force-pushed the no-local-options-in-editors branch from 477287f to f0ed7a0 Compare October 21, 2019 19:02
@tvdeyen tvdeyen merged commit d46e769 into AlchemyCMS:five Oct 21, 2019
@tvdeyen tvdeyen deleted the no-local-options-in-editors branch October 21, 2019 19:37
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.

4 participants