Allow any frontmatter property to be overridden by site.json #784

jamos-tay · 2019-03-20T08:20:40Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Enhancement to an existing feature

Fixes #647

What is the rationale for this request?

We want to be able to globally override all frontmatter properties in a page

What changes did you make? (Give an overview)

Allow any property in the frontmatter to be overriden by globalOverride if specified in the site.json,

{
  "globalOverride": {
      "arbitraryProperty": "..."
  }
}

Proposed commit message:

Allow any frontmatter property to be overridden by site.json

Some front matter properties can be overridden by site.json, but not 
all.

Let's allow any front matter property to be overriden by specifying 
them at the site.json level. Additionally, let's allow the user to 
define global front matter properties that will be applied to all 
pages.

yamgent

Propose a merge commit message.

jamos-tay · 2019-03-21T10:39:32Z

My bad, I can repush with a better message

"Allow global override of frontmatter properties"?

yamgent · 2019-03-23T02:32:50Z

My apologies, maybe I wasn't clear, but when I mean a merge commit message, it is something like this.

Basically it is a new format that we are adopting to explain the context of the change. The first paragraph is usually the current situation, the second paragraph explains why the current situation needs changes, and the third paragraph introduces our changes.

If you are strapped for time, just tell me here and I will try to come up one for you.

jamos-tay · 2019-03-23T02:56:21Z

Sure, no problem

Global override for frontmatter properties #784

Currently only certain properties such as `layout` in the frontmatter can be overridden by the site.json, but not properties such as `footer`.

We would like to allow frontmatter properties on every page to be globally overridden to give the author more flexibility. This also allows us to arbitrarily add properties to every page.

We introduce a `globalOverride` property in the site.json which is merged with every page's frontmatter, and overrides any similar properties.

jamos-tay · 2019-03-23T03:05:37Z

Rebased and updated

yamgent · 2019-03-23T14:13:44Z

Thanks for the commit message! I tidy up a bit to suit our convention better:

Add global override for frontmatter properties (#784)

Certain properties such as `layout` in the frontmatter can be overridden
by the site.json, but not properties such as `footer`. Overriding
properties also apply to only a single `page` or `glob` entry.

We would like to allow all frontmatter properties on every page to be 
globally overridden to give the author more flexibility. By providing
it as a  global option, this also allows us to add the same set of
properties and their values to every page.

Let's introduce a `globalOverride` property in the site.json which is 
merged with every page's frontmatter, and overrides any similar 
properties.

Rough description on changes made:

Message should be wrapped at 72 characters.
Title should start with imperative verbs like "Add".
Deleted "Currently" for the first paragraph - it is implied.
Added "Overriding properties also apply to only a single pageorglob entry." and rephrased the rest accordingly.
Change "We" to "Let's" for the last paragraph (this is where the commit comes in and "takes action").

On a side note,

Current: some frontmatter properties can be overridden by site.json, but not all.

Suggestion: allow any frontmatter property to be overridden by site.json

See discussion in #506

I don't think the original post expected it to be globally overridden, but rather that the individual page and glob entries can also override the properties. I know the subsequent posts did cover about global overrides, but unless I missed some discussion about that, I feel that the globals are a nice complement feature but not the full solution, so the spirit of the original post isn't really completely fulfilled by this PR?

damithc · 2019-03-23T14:22:39Z

I don't think the original post expected it to be globally overridden, but rather that the individual page and glob entries can also override the properties. I know the subsequent posts did cover about global overrides, but unless I missed some discussion about that, I feel that the globals are a nice complement feature but not the full solution, so the spirit of the original post isn't really completely fulfilled by this PR?

yes, that's correct. The original expectation is to be able to override at src/glob level, not globally.

jamos-tay · 2019-03-23T14:28:17Z

So we want something like this?

    {
      "glob": "**/index.md"
      "someProperty": ...
    }

damithc · 2019-03-23T14:33:02Z

So we want something like this?

    {
      "glob": "**/index.md"
      "someProperty": ...
    }

Yes 👍

jamos-tay · 2019-03-26T08:43:48Z

Updated

User can now specify a frontmatter property in pages:

    {
      "src": "index.md",
      "title": "Hello World",
      "frontmatter": {
        "property": "val"
      }
    },

globalOverride will override pages, pages will override frontmatter

damithc · 2019-03-26T09:08:28Z

User can now specify a frontmatter property in pages:

Nice. Ensure that if a property is specified multiple times (e.g., via multiple globs) the latest value takes precedence, based on the order given in the site.json.

globalOverride will override pages, pages will override frontmatter

An alternative is to rename this as defaultFrontmatter and let them be overridden by values specified at page level (but in site.json). That way, the user can specify the typical values as defaults and override only where some pages needs to differ. But in any case, site.json should override frontmatter in individual files, to comply with our top-down overriding policy. What do you think?

nicholaschuayunzhi

Thanks for the pr @jamos-tay! I've left some comments regarding the overriding of values. Perhaps we could make it more explicit we are using default attributes if front matter doesn't exists. Additionally, the site.json override could be a separate step that affects both branches. What do you think?

nicholaschuayunzhi · 2019-03-28T03:17:49Z

src/Page.js

-      src: this.src,
-      title: this.title || '',
-      layout: LAYOUT_DEFAULT_NAME,
+      ...{


Is there a reason why this is in an object?
Perhaps we could do

const defaultAttributes = { src: this.src, title: this.title || '', layout: LAYOUT_DEFAULT_NAME, } this.frontMatter = { ...defaultAttributes

Additionally, this could collapse with the logic above? the title check and layout check seems redundant there.

On second thought perhaps we could do something like

let front matter = { default } if (specified front matter exists) front matter = specified front matter // handle overrides

Hmm this is a bit tricky because the default src and title actually takes priority over the specified front matter, we only use the specified one if they are undefined. But we also can't overwrite specified front matter with default because overriding with ... seems to include undefineds:

a = { x: 5 } b = { x: undefined } {...a, ...b } // { x: undefined }

nicholaschuayunzhi · 2019-03-28T03:25:31Z

src/Page.js

+        layout: LAYOUT_DEFAULT_NAME,
+      },
+      ...this.frontmatterOverride,
+      ...this.globalOverride,


Here value specified by site.json, namely this.title would be overriden by frontmatterOverride and globalOverride if they specified a title. Would this be a concern?

Probably not, it was discussed here that it's easier to keep it consistent than specially handle it

Ah i see, then line 515 and 517 should be done before 512? Otherwise it would override frontmatterOverride and globalOverride which is inconsistent when front matter is not specified.

Sure, makes sense to move it down for consistency

jamos-tay · 2019-03-28T14:42:30Z

User can now specify a frontmatter property in pages:

Nice. Ensure that if a property is specified multiple times (e.g., via multiple globs) the latest value takes precedence, based on the order given in the site.json.

globalOverride will override pages, pages will override frontmatter

An alternative is to rename this as defaultFrontmatter and let them be overridden by values specified at page level (but in site.json). That way, the user can specify the typical values as defaults and override only where some pages needs to differ. But in any case, site.json should override frontmatter in individual files, to comply with our top-down overriding policy. What do you think?

Sure, makes sense. It's not too much trouble to swap the priority

src/Page.js

jamos-tay · 2019-04-13T03:28:03Z

Sorry for the late update, updated and rebased

docs/userGuide/siteConfiguration.md

yamgent · 2019-04-13T07:41:45Z

docs/userGuide/siteConfiguration.md

@@ -152,6 +159,10 @@ The following properties will apply to `index.md`:

 **An array of external scripts to be referenced on all pages.** To reference an external script only on specific pages, `externalScripts` should be specified in `pages` instead. Scripts referenced will be run before the layout script.

+#### **`globalOverride`**
+
+**Globally overrides properties in the front matter of all pages.** Any property included in the global override will automatically be merged with the front matter of every single page, and override them if the property exists. Also overrides front matter properties specified in `pages`.


This documented behaviour is no longer true? frontMatter now has a higher overriding priority than globalOverride with the latest implementation.

Also update frontMatter's description accordingly.

yamgent previously approved these changes Mar 21, 2019

View reviewed changes

jamos-tay dismissed yamgent’s stale review via c1176c1 March 21, 2019 10:30

jamos-tay force-pushed the global-override branch from de10641 to c1176c1 Compare March 21, 2019 10:30

jamos-tay force-pushed the global-override branch from c1176c1 to 612a638 Compare March 23, 2019 03:04

jamos-tay changed the title ~~Global override any front matter properties with site.json~~ [WIP] Global override any front matter properties with site.json Mar 26, 2019

jamos-tay force-pushed the global-override branch from ec3811b to 52fb236 Compare March 26, 2019 08:41

jamos-tay changed the title ~~[WIP] Global override any front matter properties with site.json~~ Global override any front matter properties with site.json Mar 26, 2019

nicholaschuayunzhi reviewed Mar 28, 2019

View reviewed changes

acjh reviewed Mar 28, 2019

View reviewed changes

src/Page.js Outdated Show resolved Hide resolved

Implement global override of frontmatter properties

5f94fdf

jamos-tay force-pushed the global-override branch from 64f447f to 5f94fdf Compare April 13, 2019 03:25

yamgent requested changes Apr 13, 2019

View reviewed changes

Fix incorrect documentation

0894091

yamgent approved these changes Apr 13, 2019

View reviewed changes

yamgent added this to the v2.1.2 milestone Apr 13, 2019

yamgent changed the title ~~Global override any front matter properties with site.json~~ Allow any frontmatter property to be overridden by site.json Apr 14, 2019

yamgent merged commit c254e7b into MarkBind:master Apr 14, 2019

yamgent mentioned this pull request Apr 14, 2019

Make overriding mechanism consistent #658

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow any frontmatter property to be overridden by site.json #784

Allow any frontmatter property to be overridden by site.json #784

jamos-tay commented Mar 20, 2019 •

edited

Loading

yamgent left a comment

jamos-tay commented Mar 21, 2019

yamgent commented Mar 23, 2019

jamos-tay commented Mar 23, 2019

jamos-tay commented Mar 23, 2019

yamgent commented Mar 23, 2019

damithc commented Mar 23, 2019

jamos-tay commented Mar 23, 2019

damithc commented Mar 23, 2019

jamos-tay commented Mar 26, 2019

damithc commented Mar 26, 2019

nicholaschuayunzhi left a comment

nicholaschuayunzhi Mar 28, 2019

nicholaschuayunzhi Mar 28, 2019

jamos-tay Mar 28, 2019 •

edited

Loading

nicholaschuayunzhi Mar 28, 2019

jamos-tay Mar 28, 2019 •

edited

Loading

nicholaschuayunzhi Mar 28, 2019

jamos-tay Apr 13, 2019

jamos-tay commented Mar 28, 2019

jamos-tay commented Apr 13, 2019

yamgent Apr 13, 2019

Allow any frontmatter property to be overridden by site.json #784

Allow any frontmatter property to be overridden by site.json #784

Conversation

jamos-tay commented Mar 20, 2019 • edited Loading

yamgent left a comment

Choose a reason for hiding this comment

jamos-tay commented Mar 21, 2019

yamgent commented Mar 23, 2019

jamos-tay commented Mar 23, 2019

jamos-tay commented Mar 23, 2019

yamgent commented Mar 23, 2019

damithc commented Mar 23, 2019

jamos-tay commented Mar 23, 2019

damithc commented Mar 23, 2019

jamos-tay commented Mar 26, 2019

damithc commented Mar 26, 2019

nicholaschuayunzhi left a comment

Choose a reason for hiding this comment

nicholaschuayunzhi Mar 28, 2019

Choose a reason for hiding this comment

nicholaschuayunzhi Mar 28, 2019

Choose a reason for hiding this comment

jamos-tay Mar 28, 2019 • edited Loading

Choose a reason for hiding this comment

nicholaschuayunzhi Mar 28, 2019

Choose a reason for hiding this comment

jamos-tay Mar 28, 2019 • edited Loading

Choose a reason for hiding this comment

nicholaschuayunzhi Mar 28, 2019

Choose a reason for hiding this comment

jamos-tay Apr 13, 2019

Choose a reason for hiding this comment

jamos-tay commented Mar 28, 2019

jamos-tay commented Apr 13, 2019

yamgent Apr 13, 2019

Choose a reason for hiding this comment

jamos-tay commented Mar 20, 2019 •

edited

Loading

jamos-tay Mar 28, 2019 •

edited

Loading

jamos-tay Mar 28, 2019 •

edited

Loading