-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add 'renderOrder' property to schema to decide in which order components render #684
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 2446
💛 - Coveralls |
@@ -71,6 +120,8 @@ function filterBaseInstanceReferences(obj) { | |||
module.exports.resolveComponentReferences = resolveComponentReferences; | |||
module.exports.composePage = composePage; | |||
module.exports.filterBaseInstanceReferences = filterBaseInstanceReferences; | |||
module.exports.referenceProperty = referenceProperty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had to export this to use it for calling resolveComponentReferences
in order to remove the breaking change for that call signature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the main thing is to rewrite listDeepValuesByKey
to be iterative instead of recursive to maintain the previous ordering.
Also like I mentioned in slack, I think we shouldn't introduce the uri at the beginning of resolveComponentReferences because it's exported from amphora - however that's more something for the amphora devs to determine.
just saw you fixed this already 👍
other than that it looks good
result = _.reduce(obj, (cumm, curr, key) => _.assign(cumm, listDeepValuesByKey(curr, filter, [...path, key])), {}); | ||
} | ||
|
||
if (_.iteratee(filter)(obj) && path.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why use iteratee here instead of _.filter(obj, filter) ?
I was wrong about _.filter because that returns an array no matter what. However I would still suggest doing something to make this more readable because _.iteratee is not a common lodash function nor is it obvious what it's doing here. So if you use it then do something like this
const passesFilter = _.iteratee(filter)
if (path.length && passesFilter(obj)) { ... }
Or more preferably separate out the function and string cases to make it more clear
const passesFilter = typeof filter === 'function'
? filter
: obj => _.get(obj, filter)
expect(result).to.have.deep.property('0.0', 'someComponent'); | ||
expect(result).to.have.deep.property('1.0', 'anotherComponent'); | ||
expect(result).to.have.deep.property('2.0', 'componentList.0'); | ||
expect(result).to.have.deep.property('3.0', 'componentList.1'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi this can be rewritten to
expect(result).to.nested.include({
'0.0': 'someComponent'
'1.0': 'anotherComponent'
'2.0': 'componentList.0'
'3.0': 'componentList.1'
})
/** | ||
* Search through an object and find all deep key-value pairs matching a filter. | ||
* @param {object} obj | ||
* @param {Function} [filter=_.identity] Optional filter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So actually since this is only called by resolveComponentReferences
and that takes a {function|string}, this should do the same.
I would just put *
in for the filter. For one, strings are passed here, but really anything that's accepted by lodash's _.filter which is just about everything can be passed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure. was just keeping the same definition as the listDeepObjects
function
* @param {array} [path] Path of this value | ||
* @returns {object} | ||
*/ | ||
function listDeepValuesByKey(obj, filter = _.identity, path = []) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the order listDeepValuesByKey results in is different from listDeepObjects due to your use of recursion (depth vs breadth first).
I tested on an article - and this matters because if you change the default order then when people update their version and rely on a certain rendering order, their code will behave differently and potentially break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey phil. tested on an article as well. the only thing that would be affected here are refs that are properties, which we already know order shouldn't be relied upon for those. array ordering is still maintained unless there is a ref inside of a ref (which afaik would never happen before data is populated)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll get an example to show what i mean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so testing against this article
listDeepObjects returns with the order of these refs
whereas listDeepValuesByKey returns with this order
In terms of keeping the public api of amphora stable, listDeepValuesByKey needs to preserve the ordering by default, regardless whether the order should be relied upon.
For instance, we were relying upon the rendering order and if that changed with a minor update, that would be a frustrating bug to find.
Ultimately it's up to the lib author to define the api and what constitutes semver changes, so if you disagree then we can let them decide.
Whoops, this PR also needs to add documentation for the new schema property. And i'm not sure, but this seems like the second schema property that affects behavior outside kiln, the other being version. I note this because the kiln docs say the schema is
so the clay team may want to keep that in mind with docs regarding the schema file |
This allows you to specify a
renderOrder
property in the_component
or_componentList
property of a layout or component's schema.yml to determine the order in which components get rendered.All component or component-lists that don't specify this property will default to
1
.Ex: