Skip to content

Swap layout override priority - #489

Merged
yamgent merged 2 commits into
MarkBind:masterfrom
jamos-tay:swap-layout-priority
Jan 20, 2019
Merged

Swap layout override priority#489
yamgent merged 2 commits into
MarkBind:masterfrom
jamos-tay:swap-layout-priority

Conversation

@jamos-tay

Copy link
Copy Markdown
Contributor

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

• [X] Other, please explain: Changing behavior of existing feature

Follow up from #467

What is the rationale for this request?

Swap the priority of loading layouts, site.json takes precedence over front matter instead of the other way around.

Added test to match.

@acjh

acjh commented Dec 22, 2018

Copy link
Copy Markdown
Contributor

I believe that was done to customize the layout for a page. The change still has to support that.

@jamos-tay

Copy link
Copy Markdown
ContributorAuthor

How should it be specified though? I'm worried while this change might be more 'correct' but we end up lose some functionality.

If the user wants to customize the page then he should probably not use globs in the site.json

@acjh

acjh commented Dec 22, 2018

Copy link
Copy Markdown
Contributor

Oh, it's already customizable in site.json.

Comment threaddocs/userGuide/pageLayout.md Outdated
Comment threadtest/test_site/testLayoutsOverride.md Outdated
Comment threadsrc/Page.js Outdated
@jamos-tay

Copy link
Copy Markdown
ContributorAuthor

Updated

</frontmatter>

# Uses a layout No newline at end of file
# Uses a front matter layout No newline at end of file

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In site.json, no layout is specified.
In the frontmatter, testLayout is specified.
The final output still uses default instead of testLayout.

That doesn't seem right. 😕

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It feels like the default should have the lowest priority.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

My bad, fixed it, there was some issue with the glob layouts

@jamos-tay

Copy link
Copy Markdown
ContributorAuthor

Updated and Rebased

@yamgentyamgent added this to the v1.15.3 milestone Jan 1, 2019
@yamgent

Copy link
Copy Markdown
Member

@jamos-tay The documentation needs to be updated.

@yamgentyamgent removed this from the v1.15.3 milestone Jan 3, 2019
@jamos-tay
jamos-tayforce-pushed the swap-layout-priority branch from e041dc6 to eb260bfCompareJanuary 8, 2019 05:57
@jamos-tay

Copy link
Copy Markdown
ContributorAuthor

Updated and rebased on master

@yamgentyamgent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please squash the commits.

@jamos-tay

Copy link
Copy Markdown
ContributorAuthor

Squashed and rebased

@yamgentyamgent added this to the v1.16.2 milestone Jan 15, 2019
Comment threadtest/test_site/site.json Outdated
@yamgentyamgent removed this from the v1.16.2 milestone Jan 17, 2019
@jamos-tay

Copy link
Copy Markdown
ContributorAuthor

Updated

@yamgentyamgent added this to the v1.16.2 milestone Jan 19, 2019
@yamgent
yamgent merged commit ad65565 into MarkBind:masterJan 20, 2019
@nicholaschuayunzhi

nicholaschuayunzhi commented Jan 21, 2019

Copy link
Copy Markdown
Contributor

I have a diff when doing file compare for siteData.json. Red is expected, and green is what markbind generated.

{
"headings": {
- "uses-a-site.json-layout%2C-overriding-front-matter": "Uses a site.json layout, overriding front matter"+ "uses-a-site-json-layout-overriding-front-matter": "Uses a site.json layout, overriding front matter"
},
"title": "Hello World",
"head": "overwriteLayoutHead.md",
"layout": "testLayout",
"src": "testLayoutsOverride.md"
},

Want to clarify if the line in the expected file is correct.

@acjh

acjh commented Jan 21, 2019

Copy link
Copy Markdown
Contributor

I believe expected needs to be updated for #578.

@yamgent

Copy link
Copy Markdown
Member

@nicholaschuayunzhi thanks for spotting the error, my apologies for the error, I didn't re-run the tests when merging this PR.

The fix is now available on the master branch, in commit f02c338.

Sign up for freeto 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.

5 participants

@jamos-tay@acjh@yamgent@nicholaschuayunzhi@damithc