Added info about registration pages numeration #769
Conversation
wickathou
left a comment
wickathou
left a comment
There was a problem hiding this comment.
Hi @nikkxll ,
Great work! I only got a couple of points that would make the changes you made more stable and understandable for other devs
Required Changes ♻️
Check the comments under the review.
Optional suggestions
Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better.
Consider also:
- Exploring the topic of Git flow, as it makes your repo easier to work with and collaborate with other. If you make your changes directly on your STAG branch, if then you want to sync it with the hackjunction STAG branch, there could be conflicts which would prevent you from syncing - Atlassian's gitflow workflow
Feel free to leave any questions or comments in the PR thread mentioning me so I can see the notification and reply @wickathou.
Cheers and Happy coding!👏👏👏
Please, do not open a new Pull Request for re-reviews.
| <Typography className={classes.sectionsNum}> | ||
| {`${sectionsInfo.index}/${sectionsInfo.sections}`} | ||
| </Typography> |
There was a problem hiding this comment.
Important
It would be better if you add a verification to make sure sectionsInfo exist, so if 'RegistrationBottomBar' is implemented in another context, where it doesn't get passed the sectionsInfo prop, then the component would throw an exception
An example would be:
{sectionsInfo && (<Typography...}
| sectionsInfo={{ | ||
| sections: sections.length, | ||
| index: index + 1, | ||
| }} |
There was a problem hiding this comment.
Optional
Since the 'index' property, is going to refer to the active section, you should change the property name to something more descriptive like for example activeSection
wickathou
left a comment
wickathou
left a comment
There was a problem hiding this comment.
Hi @nikkxll ,
Great work! 🎉
Your PR is scheduled for merge on February 27th, so if you want to implement any update before, feel free to do it
Optional suggestions
- Check the comments
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.
| sectionsInfo={{ | ||
| sections: sections.length, | ||
| activeSection: index + 1, | ||
| }} |
There was a problem hiding this comment.
Optional
Since this object sectionsInfo is passed as a prop for the RegistrationSection component on lin 347, it would be good to write once, and pass the same object to both components, as to avoid differences between both cases
2 participants
No description provided.