Improve meta fields #538

tilsammans · 2016-06-17T15:41:37Z

This pull request adds default fields for author, publisher and keywords to Theming (Account).

houndci-bot · 2016-06-17T15:41:47Z

spec/models/pageflow/entry_spec.rb

+
+        entry = create(:entry)
+
+        expect(entry.draft.author).to eq("Prof. Dr. Sahra Isak")


Prefer single-quoted strings when you don't need string interpolation or special symbols.

tf · 2016-06-17T16:12:48Z

app/assets/javascripts/pageflow/editor/views/edit_meta_data_view.js

+      });
+      this.input('keywords', pageflow.TextInputView, {
+        placeholder: pageflow.theming.attributes.default_keywords
+      });


Now that values are copied, displaying placeholders here might be misleading. Clearing the text box does not reset the values to the theming default.

I agree. Drop them completely? Or replace with some generic placeholder?

Author Publisher comma, separated, keywords

Looking at the meta tag helper it looks like we are actually falling back to global config, not the theming default when the user clears an input here. So I guess, we should be should be displaying that as a placeholder.

I believe that's what my latest change accomplished.

But it would still display theming.default_author if that is present or am I missing something?

This goes back to our discussion about copying vs "inheriting" defaults. If we extend the fallback logic of the helper, I'm not sure whether things might get more confusing since there then can be

entries with copied default meta data which do not change when the defaults in the theming change

entries with emptied meta data attributes which then fallback to theming defaults but also change when the theming changes

Being able to retroactively change the meta data of a lot of entries by editing the theming seems like to much power to me. I'd say let's just display the defaults form the global config as placeholders here. Then if somebody chooses to use those by emptying his meta data fields, at least there is less risk of unexpected change because changing these defaults at least means editing the initializer and deploying. Makes sense?

Right. I knew there was something.

Fixed in e9f6f66.

I will squash & rebase now.

One last thing: Let's pass the global default values via app/views/pageflow/config/_editor_seeds.json.jbuilder

Using the theming as a transport might trip us up later, since those are not really the theming attributes we are passing.

Attributes rendered in the config seed are available as pageflow.config.defaultSomething inside the editor js.

houndci-bot · 2016-06-25T09:24:53Z

spec/models/pageflow/theming_spec.rb

+
+        theming.copy_defaults_to(revision)
+
+        expect(revision.keywords).to eq("ratione, aut, blanditiis")


Prefer single-quoted strings when you don't need string interpolation or special symbols.

coveralls · 2016-06-25T09:29:06Z

Changes Unknown when pulling c31dd02 on scrollytelling:default-meta-on-theming into * on codevise:master*.

coveralls · 2016-06-25T09:40:11Z

Changes Unknown when pulling 803025e on scrollytelling:default-meta-on-theming into * on codevise:master*.

tilsammans · 2016-06-25T09:59:03Z

@tf pushed up the required changes. Review?

coveralls · 2016-06-25T10:02:38Z

Changes Unknown when pulling 8bec49b on scrollytelling:default-meta-on-theming into * on codevise:master*.

tf · 2016-06-27T09:21:58Z

app/models/pageflow/theming.rb

+        publisher: default_publisher.presence || Pageflow.config.default_publisher_meta_tag,
+        keywords: default_keywords.presence || Pageflow.config.default_keywords_meta_tag
+      )
+    end


I think Hound is right here. Let's keep the Widget.copy_all_to and extract the revision update into a copy_default_meta_tags method. That way copy_defaults_to has a nicer abstraction level.

tilsammans · 2016-06-27T09:25:23Z

I can continue tomorrow at the earliest.

tilsammans · 2016-06-28T06:50:59Z

Widget.copy_all_to is now retained, and rebased on master.

coveralls · 2016-06-28T06:54:46Z

Coverage increased (+0.01%) to 97.638% when pulling 33dc47f on scrollytelling:default-meta-on-theming into cad8bf5 on codevise:master.

tf · 2016-06-28T09:16:22Z

spec/models/pageflow/widget_spec.rb

-        expect(revision.widgets).to include_record_with(type_name: 'custom_header', role: 'header')
-      end
-    end
-


We lost a spec here.

Oops, forgot to paste it back. 😟

This has been added back in.

tf · 2016-06-28T09:25:02Z

Looks good! Can you address the last remaining points and squash the commits? Then we're good to merge.

tilsammans · 2016-06-28T09:28:59Z

I will address them.

As far as squishing, this is now something built-in to GitHub. It works quite well! You can select the "squash commits" option on the merge button. I will update the PR probably later today.

tf · 2016-06-28T09:33:29Z

Interesting. To be honest, I actually like the merge commits though since they state the PR number and branch name. Those would not show up if I hit the squash button, right? I just don't need the fine grain commit history that lead to the final state of the PR on master.

tilsammans · 2016-06-28T09:39:13Z

Those would not show up if I hit the squash button, right?

PR number does, in parentheses. And each commit message is added to a bullet list in the merge description. It's pretty nice. I'll squash-merge your PR for our navigation plugin so you can see if you like it. Otherwise happy to squash myself.

tilsammans · 2016-06-28T12:02:59Z

this is what a GitHub-squashed commit looks like. If there had been more than one commit, it would all have become just one.

tf · 2016-06-28T13:55:58Z

I might try this on a new project, but for consistency I think I will stick with merging here. So it would be great if you could squash once your done. Thanks.

coveralls · 2016-06-29T09:14:52Z

Coverage increased (+0.05%) to 97.673% when pulling 2bc90b3 on scrollytelling:default-meta-on-theming into cad8bf5 on codevise:master.

coveralls · 2016-06-29T09:21:41Z

Coverage increased (+0.06%) to 97.685% when pulling 05feb74 on scrollytelling:default-meta-on-theming into cad8bf5 on codevise:master.

coveralls · 2016-06-29T09:32:59Z

Coverage increased (+0.08%) to 97.706% when pulling e9f6f66 on scrollytelling:default-meta-on-theming into cad8bf5 on codevise:master.

tilsammans · 2016-06-29T09:47:50Z

moved meta defaults to global JS config in 598e32a.

coveralls · 2016-06-29T09:49:57Z

Coverage increased (+0.01%) to 97.643% when pulling 598e32a on scrollytelling:default-meta-on-theming into cad8bf5 on codevise:master.

coveralls · 2016-06-29T09:57:35Z

Coverage increased (+0.06%) to 97.685% when pulling 47a9e93 on scrollytelling:default-meta-on-theming into cad8bf5 on codevise:master.

* default_author * default_publisher * default_keywords These will be taken to become author, publisher and keywords fields for new entries for this Theming. Also render those defaults as placeholders on the Entry fields. We copy Configuration meta defaults into Revision. This way, we will persist those metas with the revision, and they will always be part of that publication. Given a new revision, these meta fields can be changed, or removed. fallback to Config meta defaults in the editor Also show placeholders in Theming edit form render config values as Theming defaults This goes back to our discussion about copying vs "inheriting" defaults. If we extend the fallback logic of the helper, I'm not sure whether things might get more confusing since there then can be entries with copied default meta data which do not change when the defaults in the theming change entries with emptied meta data attributes which then fallback to theming defaults but also change when the theming changes Being able to retroactively change the meta data of a lot of entries by editing the theming seems like to much power to me. I'd say let's just display the defaults form the global config as placeholders here. Then if somebody chooses to use those by emptying his meta data fields, at least there is less risk of unexpected change because changing these defaults at least means editing the initializer and deploying. Makes sense?

coveralls · 2016-06-29T10:48:20Z

Coverage increased (+0.02%) to 97.643% when pulling c2772b8 on scrollytelling:default-meta-on-theming into a90d696 on codevise:master.

tilsammans · 2016-06-29T15:30:11Z

🎉

houndci-bot reviewed Jun 17, 2016
View reviewed changes

tilsammans mentioned this pull request Jun 17, 2016

Improve configurable meta tags #512

Closed

tf reviewed Jun 17, 2016
View reviewed changes

tf changed the title ~~Improve meta fields~~ [WIP] Improve meta fields Jun 22, 2016

houndci-bot reviewed Jun 25, 2016
View reviewed changes

tf reviewed Jun 27, 2016
View reviewed changes

tf reviewed Jun 28, 2016
View reviewed changes

tf changed the title ~~[WIP] Improve meta fields~~ Improve meta fields Jun 29, 2016

tf modified the milestone: v0.11 Jun 29, 2016

tf merged commit 418f90c into codevise:master Jun 29, 2016

tilsammans deleted the default-meta-on-theming branch June 29, 2016 15:30


		entry = create(:entry)

		expect(entry.draft.author).to eq("Prof. Dr. Sahra Isak")


		theming.copy_defaults_to(revision)

		expect(revision.keywords).to eq("ratione, aut, blanditiis")

Improve meta fields #538

Improve meta fields #538

Conversation

tilsammans commented Jun 17, 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

tf Jun 29, 2016 • edited Loading

Choose a reason for hiding this comment

tf Jun 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 25, 2016 • edited Loading

coveralls commented Jun 25, 2016 • edited Loading

tilsammans commented Jun 25, 2016

coveralls commented Jun 25, 2016 • edited Loading

tf Jun 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tilsammans commented Jun 27, 2016

tilsammans commented Jun 28, 2016

coveralls commented Jun 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tf commented Jun 28, 2016

tilsammans commented Jun 28, 2016

tf commented Jun 28, 2016

tilsammans commented Jun 28, 2016

tilsammans commented Jun 28, 2016

tf commented Jun 28, 2016

coveralls commented Jun 29, 2016 • edited Loading

coveralls commented Jun 29, 2016 • edited Loading

coveralls commented Jun 29, 2016 • edited Loading

tilsammans commented Jun 29, 2016

coveralls commented Jun 29, 2016 • edited Loading

coveralls commented Jun 29, 2016 • edited Loading

coveralls commented Jun 29, 2016 • edited Loading

tilsammans commented Jun 29, 2016

tilsammans commented Jun 17, 2016 •

edited

Loading

tf Jun 29, 2016 •

edited

Loading

tf Jun 29, 2016 •

edited

Loading

coveralls commented Jun 25, 2016 •

edited

Loading

coveralls commented Jun 25, 2016 •

edited

Loading

coveralls commented Jun 25, 2016 •

edited

Loading

tf Jun 27, 2016 •

edited

Loading

coveralls commented Jun 28, 2016 •

edited

Loading

coveralls commented Jun 29, 2016 •

edited

Loading

coveralls commented Jun 29, 2016 •

edited

Loading

coveralls commented Jun 29, 2016 •

edited

Loading

coveralls commented Jun 29, 2016 •

edited

Loading

coveralls commented Jun 29, 2016 •

edited

Loading

coveralls commented Jun 29, 2016 •

edited

Loading