Closed localjo closed 8 years ago
I broke a couple tests, so I need to fix those, and then write a test for this feature.
Changes look good, but I'm wondering if we should pick a more specific keyword than preview
, which could happen with real URLs?
Do you mean a more specific query string? A preview situation could happen on an existing URL where there are changes to a post that someone wants to preview.
Yeah a different query string than preview
, just in case that query string actually exists on a real resource.
Well the query string is for the Solidus site, the resources don't get that preview string added.
Unless one of them is dynamic and uses the preview
query string value. It's an edge case, but it could happen.
So, basically if there's a resource with {preview}
as a dynamic segment, it could conflict? Any suggestions on a better name? At best we're just making a collision less likely, but is it likely enough that we need to worry about it? I grepped all of my local copies of solidus sites and didn't see any resources that use {preview}
as a dynamic segment.
Right, as I was saying it's an edge case, I guess if it ever happens we can always rename the dynamic segment... Maybe use preview_page
?
Also, I think we should also ignore preprocessor errors when in preview mode. Most preprocessors are used to transform resources, and the page will likely blow up if the resource is missing.
Since we're talking about this as a "mode" how about following the boolean convention?
isPreview=true
Or use Yoda casing: previewIs=true
I originally had isPreview=true
, but Joannic said the Solidus convention is to use underscore casing for variables, and camel casing for methods.
Yeah actually for query strings I think underscores are better. Less concerned about case, more concerned about wording =)
But we really ought to switch this project over to semistandard sometime soon. I don't know how I ever lived with all this padding! Then again, I've never contributed any code to solidus!
Wait, are we referring to the query string in the URL or the context.is_preview
property?
I'm talking about the query string.
Ok, updated that to isParam
throughout all of these PRs.
In my first comment, I was talking about the isPreview
variable name, with the wrong casing... :P
Yeah, noticed that now. :stuck_out_tongue:
Is this good to merge into master and do a release so we don't have to lock package.json files to this branch?
Before merging, and introducing the new preview mode, I suggest:
isOptional
variable into is_optional
isPreview
query parameter into is_preview
How's that look?
Looking good, but I'd change the req.query.isPreview
param to req.query.is_preview
(for consistency).
Oops, sorry, got mixed messages about that. It's is_preview
everywhere now. Good to go?
:+1: (please squash your commits before merging!)
Allow certain query parameters to trigger a "preview mode" where all resources are optional.