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
Enhancing LinWei's Paging construct #34
Enhancing LinWei's Paging construct #34
Conversation
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.
Overall, implementation was clean and clear. Great work.
I would personally prefer the pages being non static and that the classes would need a pages object in order to call it, this would minimise instances where the List<Page> pages
could be changed if you can have access to getPages()
from anywhere if it is static simply by importing the class. But making it static is fine also!
I would also suggest that you create some test classes to increase testability. In addition, try to run checkStyleMain()
, although we agreed to disable it but you wouldn't want to accumulate too much checkStyles to resolve.
Last but not least, ready to merge once you're ready (after passing Travis)!
Apart from making my FinancialTrackerPage being able to switch between main and financial tracker page smoothly, I've also implemented ways to load FXML files easily. You guys can easily deal with SceneBuilder afterwards. With that being said, do look around and tell me if you think it's okay :P
Do check out my FinancialTrackerPage.java constructor