Content blocks should be editable in settings UI #82

mjgiarlo · 2016-04-15T00:57:10Z

Fixes #74

cbeer · 2016-04-21T21:55:59Z

app/helpers/sufia_helper.rb

+
+  def displayable_content_block(content_block, class_string = '', id_string = '')
+    return if content_block.value.blank?
+    raw("<div id='#{id_string}' class='#{class_string}'>#{content_block.value}</div>")


Maybe:

content_tag :div, content_block.value, id: id_string, class: class_string

Instead of passing the class and id, how about we pass options: {} and pass it through to the content_tag?

houndci-bot · 2016-04-22T02:23:27Z

app/assets/stylesheets/batch_edit.scss

@@ -0,0 +1 @@
+@import 'batch_edit/batch_edit';


Prefer double-quoted strings

cbeer · 2016-04-22T03:37:25Z

.rubocop.yml

@@ -24,6 +24,9 @@ Metrics/LineLength:
  Exclude:
    - 'app/controllers/catalog_controller.rb'

+Metrics/AbcSize:
+  Exclude:
+    - 'app/helpers/content_block_helper.rb'


Instead of excluding this, maybe we can figure out how to simplify those methods?

It's the editable_content_block method in particular, which has a score of 16 (the threshold is 15). Extracting form_for into its own method, like you suggested, may do the trick. I'll have a go at this later.

Killed this method.

houndci-bot · 2016-04-25T23:21:25Z

spec/views/sites/edit.html.erb_spec.rb

@@ -4,6 +4,15 @@
  let!(:site) do
    assign(:site, Site.create!(application_name: "MyString", institution_name: "My Inst Name"))
  end
+  let(:announcement_text) { ContentBlock.find_or_create_by(name: ContentBlock::ANNOUNCEMENT)}
+  let(:marketing_text) { ContentBlock.find_or_create_by(name: ContentBlock::MARKETING)}
+  let(:featured_researcher) { ContentBlock.find_or_create_by(name: ContentBlock::RESEARCHER)}


Style/SpaceInsideBlockBraces: Space missing inside }.

jcoyne · 2016-04-26T01:02:17Z

app/models/site.rb


    def instance
      first || create
    end
  end
+
+  def announcement_text


IMHO these methods do not belong on the site model. You should extract this stuff into a Form object which is responsible for saving the site and any content blocks.

I'd like to defer creating a new form object, if at all possible.

We're getting ContentBlock from sufia, I imagine? Instead of adding our own accessors here, would it make sense to set up the expected AR associations to that model?

@cbeer Correct, that model is provided by Sufia. Do you have something like the following in mind?

class Site < ActiveRecord::Base # ... has_many :content_blocks # ... class << self delegate :account, ..., :content_blocks, to: :instance delegate :announcement_text, :marketing_text, :featured_researcher, to: :content_blocks # ... end end

If so, I'll need to create some convenience methods in Sufia (#announcement_text, #marketing_text, #featured_researcher) first.

Fixes #74

mjgiarlo added the in progress label Apr 15, 2016

cbeer reviewed Apr 21, 2016
View reviewed changes

mjgiarlo changed the title ~~[WIP] Shameless green version of content blocks editable in settings UI.~~ Content blocks should be editable in settings UI Apr 21, 2016

mjgiarlo force-pushed the add_content_blocks_to_settings branch 3 times, most recently from 828c031 to 5195eaa Compare April 22, 2016 02:23

houndci-bot reviewed Apr 22, 2016
View reviewed changes

app/assets/stylesheets/batch_edit.scss

@@ -0,0 +1 @@

@import 'batch_edit/batch_edit';

Copy link

houndci-bot Apr 22, 2016

Choose a reason for hiding this comment

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

Prefer double-quoted strings

mjgiarlo force-pushed the add_content_blocks_to_settings branch from 5195eaa to 8196b60 Compare April 22, 2016 02:31

mjgiarlo changed the title ~~Content blocks should be editable in settings UI~~ [WIP] Content blocks should be editable in settings UI Apr 22, 2016

cbeer reviewed Apr 22, 2016
View reviewed changes

cbeer force-pushed the add_content_blocks_to_settings branch from 8196b60 to 6866e25 Compare April 22, 2016 15:44

mjgiarlo force-pushed the add_content_blocks_to_settings branch from 6866e25 to d0f27f0 Compare April 25, 2016 23:21

houndci-bot reviewed Apr 25, 2016
View reviewed changes

mjgiarlo force-pushed the add_content_blocks_to_settings branch 4 times, most recently from c419570 to 0015b01 Compare April 26, 2016 00:33

mjgiarlo changed the title ~~[WIP] Content blocks should be editable in settings UI~~ Content blocks should be editable in settings UI Apr 26, 2016

jcoyne reviewed Apr 26, 2016
View reviewed changes

mjgiarlo force-pushed the add_content_blocks_to_settings branch from 0015b01 to 21c2df1 Compare April 26, 2016 18:06

mjgiarlo mentioned this pull request Apr 26, 2016

Add convenience methods for content blocks samvera-deprecated/sufia#1927

Merged

Make content blocks editable in settings UI

d38bc66

Fixes #74

mjgiarlo force-pushed the add_content_blocks_to_settings branch from 21c2df1 to d38bc66 Compare April 26, 2016 22:22

atz merged commit 9ff1100 into master Apr 26, 2016

atz deleted the add_content_blocks_to_settings branch April 26, 2016 23:44

atz removed the in progress label Apr 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Content blocks should be editable in settings UI #82

Content blocks should be editable in settings UI #82

mjgiarlo commented Apr 15, 2016 •

edited

Loading

cbeer Apr 21, 2016

cbeer Apr 21, 2016

houndci-bot Apr 22, 2016

cbeer Apr 22, 2016

mjgiarlo Apr 22, 2016

mjgiarlo Apr 25, 2016

houndci-bot Apr 25, 2016

jcoyne Apr 26, 2016

mjgiarlo Apr 26, 2016

cbeer Apr 26, 2016

mjgiarlo Apr 26, 2016 •

edited

Loading

Content blocks should be editable in settings UI #82

Content blocks should be editable in settings UI #82

Conversation

mjgiarlo commented Apr 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjgiarlo Apr 26, 2016 • edited Loading

Choose a reason for hiding this comment

mjgiarlo commented Apr 15, 2016 •

edited

Loading

mjgiarlo Apr 26, 2016 •

edited

Loading