Restructured to be closer to HTML partials version.#896
Restructured to be closer to HTML partials version.#896alankent wants to merge 2 commits intoampproject:ampstart-2.0from
Conversation
sebastianbenz
left a comment
sebastianbenz
left a comment
There was a problem hiding this comment.
I think it's already much better. Left a few comments.
| <div className='overflow-scroll'> | ||
| <div className='travel-overflow-container'> | ||
| <div className='flex justify-center p1 md-px1 mxn1'> | ||
| {[ |
There was a problem hiding this comment.
suggestion: extract the list into a variable
There was a problem hiding this comment.
Seems also to duplicate TravelData
There was a problem hiding this comment.
Some I converted from result of converter and then reverse engineered the source template. Next one I think should port the Mustache files rather than the output of the conversion. There are few like this around the place I have not done yet.
| @@ -0,0 +1,202 @@ | |||
| export default function TravelData() { | |||
There was a problem hiding this comment.
This feels very hard to read and it also seems to mix different kinds of data (domain and presentational data). I think it'd be better to split this up into multiple files.
There was a problem hiding this comment.
This was copied out of the original template - I have not got to the bottom of the two sets of data - feels like some overlap there as well. Let's work out what we want as components to be listed separately - that might also help clean all this up as well.
| 'head-tag': { | ||
| title: 'Travel Template', | ||
| 'canonical-path': 'templates/travel/travel.amp', | ||
| extensions: ['amp-list', 'amp-bind'], |
There was a problem hiding this comment.
this is no longer needed
| }, | ||
| 'head-tag': { | ||
| title: 'Travel Template', | ||
| 'canonical-path': 'templates/travel/travel.amp', |
There was a problem hiding this comment.
this isn't needed either
| @@ -0,0 +1,146 @@ | |||
| export default function TravelResultsData() { | |||
There was a problem hiding this comment.
general question: do we want to shorten default exports to export default () => { ... }?
| export default function AmpListProps(includeDates) { | ||
| return { | ||
| src: | ||
| '/api/search?maxPrice=0' + |
There was a problem hiding this comment.
Use URL and URL.searchParams to create the string instead. It's much more readable.
| @@ -0,0 +1,16 @@ | |||
| export default function AmpListProps(includeDates) { | |||
There was a problem hiding this comment.
Maybe find a better name for this function / file. It's not a util.
templates/travel/pages/api/foo.js
Outdated
| * @param {Object} res - The HTTP response object. | ||
| * @param {Function} next - The next HTTP handler to execute. | ||
| */ | ||
| function cors(req, res, next) { |
There was a problem hiding this comment.
use @ampproject/toolbox-cors instead.
| <AmpState id='fields_maxPrice_edited'>{false}</AmpState> | ||
| <AmpState id='query_maxPrice'>{801}</AmpState> | ||
| </div> | ||
| <AmpState id='display'> |
There was a problem hiding this comment.
I'm not a fan of namespacing via name as it results in unnecessary redundancy. Instead I'd prefer:
<AmpState id="ui">{{
hero: true,
reset: false,
...
}}<AmpState/>
| <link rel="stylesheet" type="text/css" href="/css/travel-results.css" /> | ||
| </Head> | ||
|
|
||
| <AmpState id="display"> |
There was a problem hiding this comment.
this feels redundant
|
Alan Kent seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
3 participants
This PR is not intended to be merged.
This still has lots of data-amp-bind-* in it, without loading up AmpBind. But it shows progress of renaming all the components to align with the HTML partials version.
It also moves all the AMP State variables from global state to a single nested group. This is not the final version - it should have multiple global namespaces (not one per variable, not one for everything).
@sebastianbenz