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
Enable Transactions page SSR #4753
Conversation
Your Render PR Server URL is https://oc-styleguide-pr-4753.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-bskpta2pp1jjdjar50s0. |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/opencollective/opencollective-frontend/nrmh8yjio |
f85840f
to
29f73c4
Compare
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.
Let's try to make the way props and variables are flowing clearer, renaming transactionsQuery
will be a good first step. Interested to see @Betree feedback.
pages/transactions.js
Outdated
export default withUser(addCollectiveCoverData(TransactionsPage)); | ||
const addTransactionsData = graphql(transactionsQuery, { | ||
skip: props => !props.slug, | ||
name: 'transactionsQuery', |
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.
This is not a good name, it's confusing.
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 agree that it creates confusion with the query itself, something like transactionsQueryResult
would make more sense to me.
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 renamed it to transactionsData
.
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.
Looks better!
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 like that we're moving the query to the page, I think it's more suited there. In the same idea, having the pushRoute
in the page would make this component usable on other pages if ever needed.
Beside the variable name mentioned above, the rest looks good to me!
pages/transactions.js
Outdated
@@ -106,4 +213,15 @@ class TransactionsPage extends React.Component { | |||
} | |||
} | |||
|
|||
export default withUser(addCollectiveCoverData(TransactionsPage)); | |||
const addTransactionsData = graphql(transactionsQuery, { | |||
skip: props => !props.slug, |
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.
collectiveSlug
is required in
opencollective-frontend/server/pages.js
Line 41 in a34c29a
.add('transactions', '/:parentCollectiveSlug?/:collectiveType(events|projects)?/:collectiveSlug/transactions') |
Related to opencollective/opencollective#3385