Conversation
fabiosantoscode
left a comment
fabiosantoscode
left a comment
There was a problem hiding this comment.
Looks great in general!
Don't feel obligated to change stuff marked as a nitpick.
pages/api/blog.js
Outdated
|
|
||
| res.status(200).json(data) | ||
| } catch { | ||
| res.status(404) |
There was a problem hiding this comment.
Is this the appropriate status code? Maybe 503/502 is better since it's an upstream error.
There was a problem hiding this comment.
You are right, will change.
pages/community.js
Outdated
| /> | ||
| <title>Community | {META_BASE_TITLE}</title> | ||
| </Head> | ||
| ) |
There was a problem hiding this comment.
nitpick: this could be a head and used below as {head} since it's static.
Don't feel obligated to change this if you feel like this is a better solution.
There was a problem hiding this comment.
I actually moved it inside the component on the other pages already, so probably I'll do it here too.
| // eslint-disable-next-line react/display-name | ||
| export default React.forwardRef((props, ref) => ( | ||
| <BareButton {...props} forwardedRef={ref} /> | ||
| )) |
There was a problem hiding this comment.
I think you can make the BareButton a wrappee of React.forwardRef itself, instead of adding an extra component around it. It should work fine even if you add hooks to the component eventually.
There was a problem hiding this comment.
It's mostly done this way to let the button have a readable name to show in dev tools, bow I start to think that I can do both without separate component, but I need to experiment some more.
There was a problem hiding this comment.
Tried, didn't work (you can't default export consts, only functions). But I renamed BareButton to CommunityButton for consistency.
| <CommunityButton | ||
| theme={theme} | ||
| href="mailto:info@dvc.org?subject=I want to write a blogpost!" | ||
| target="_blank" |
There was a problem hiding this comment.
nitpick: the only way of making mailto links open in a new tab is using window.open.
There was a problem hiding this comment.
It's ok not to open new page if the user use external email app. We need it only for browser email client like gmail.
@shcheklein can you check if the links are now opened in the new tab in your case please?
jorgeorpinel
left a comment
jorgeorpinel
left a comment
There was a problem hiding this comment.
From #874 (comment) I still have these 2 Qs:
- Is removing the GitHub link from the top nav a good idea?
- The Be an Ambassador block may need more emphasis, maybe a special color and/or being on top of its section.
Elle's copy edits which I moved to #874 (review) seem to all be pending still.
My own #874 (review) on src/Community/data.json also seems pending still.
|
@iAdramelk looks like a lot of good reviews and suggestions were lost due to a lot of changes. @SvetaGr @jorgeorpinel @andronovhopf could you summarize in one page, please, what copy updates do you suggest to make? @jorgeorpinel if you have anything related to code - could you please make a separate issue with checkboxes to discuss and fix them? Thanks! |
|
I'll just apply the copy edits in a regular update PR soon, so don't worry about my #874 (review) above Alex. Sorry, I didn't really get a chance to review the JS code here Ivan, my review on Thanks |
|
@jorgeorpinel in this specific case, let's create a list of changes please since @andronovhopf and @SvetaGr had some changes in mind as well. I would like to consolidate this and do at once. |
|
OK let's use #996 |
7 participants
Strongly under development. PR only to check on Heroku.Purpose:
Motivation: