COR-217: Content Publishable #341
Conversation
| private | ||
|
|
||
| def render_edit | ||
| link_to "<i class='material-icons'>create</i>".html_safe, edit_content_type_content_item_path(content_type.id, content_item.id), class: 'mdl-button mdl-js-button mdl-button--icon' |
There was a problem hiding this comment.
I'd recommend we move this to the view, especially since we're executing html_safe here
| @@ -0,0 +1,7 @@ | |||
| module Buttons | |||
There was a problem hiding this comment.
I'm not sure how I feel about Publishable buttons as cells, especially since all the logic for these buttons is going to be client-side (i.e. when you change published date in the interface, the data-bound button will update to reflect the change in state). At the very least, I think the namespacing for these Cells is iffy - instead of a Buttons namespace, let's go with PublishableNav::DraftButtonCell or something similar - if we used Buttons for every button cell, which the namespacing implies, we'd have a ton of objects in this namespace.
| end | ||
|
|
||
| def content_item | ||
| @model |
There was a problem hiding this comment.
If you want access to the model, use Cell's accessor method, model, rather than the instance var. This is generally considered safer in OOP, as it allows you to add functionality to the getter/setter at a later date that you'd otherwise have to use hooks to accomplish with an instance variable.
There was a problem hiding this comment.
Additionally, generally, I believe we're breaking Cells by making our model be something other than what our Cell is trying to represent. To me, this is a smell - that we may be too granular in our use of cells, or that we should be passing this data in the options or context hashes, and passing nil as our model, if the use case is valid. In the past, you've had a valid use case for this - the Tree system needed the granularity of cells with nil models.
| end | ||
|
|
||
| def publish_state | ||
| "Published" | ||
| def get_publish_state |
There was a problem hiding this comment.
Any reason we're favoring the get_ prefix here? I think this is a bit against the Ruby/Rails grain - getters usually are written as what they return, which in this case is just publish_state, as it was before.
There was a problem hiding this comment.
This was done because #publish_state calls the PublishState object, which existed at the time
ElliottAYoung
force-pushed
the
topic/COR-217-Content-Publishable
branch
from
5992bc2
to
409727f
Compare
ElliottAYoung
force-pushed
the
topic/COR-217-Content-Publishable
branch
from
47bff9e
to
2806945
Compare
|
@arelia @toastercup |
|
@toastercup @arelia |
| @@ -0,0 +1,15 @@ | |||
| class ButtonsCell < Cell::ViewModel | |||
There was a problem hiding this comment.
I'm not sure this is a good candidate for an entire class.. I'd just nest it in the view, since it's not complex business logic nor does it utilize its own model
@arelia @toastercup
Paired with @arelia on this one
Adds the
publishableattribute to theContentTypefield so thatContentTypes can be dynamically flagged to be publishable or not. From there we have created thePublishStatetable to create and update the publish state and all relevant metadata for allContentItems wherepublishableis set to true. We have adjusted the buttons on the form to be set based on the currentPublishStateof theContentItemand have extracted these into cells.PublishStateis now actually retrievable on the Index using the#get_publish_statemethod on theContentItemmodel.