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
Bug fixes and new features #4
Conversation
|
Code is OK, but two commit messages refer to the wrong bugs; instructions for fixing those are above. In the future, it's a lot easier to keep track of reviews when there is separate review for each change/bug; when there are five or six bunched together like this, it's confusing to test and comment on. If you use feature branches, it's much easier to do :) |
| import QtQuick 1.1 | ||
| import com.nokia.meego 1.0 | ||
|
|
||
| Item{ |
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.
styling related (I'd prefer to see this fixed, I know we don't yet have nemo-wide recommendations, but we should) is generally that there's a space after Item before {
|
just some style nitpicking. I'll trust john on the content of the change, and we'll merge this as-is. I'd appreciate to see a fixup commit for the style issues later (and I'll go off and start figuring out that recommendations guide..). |
|
That is, it can be merged as-is once the commit messages are fixed. |
… of the device. To do that, the view which shows fullscreen images has been completely rewritten. ImageContainer QML component has been added, .qrc file edited to add ImageContainer.qml
| if (rect.width == imgController.width) { | ||
|
|
||
| //Anchors the image to the left | ||
| if (flickImg.contentX < 0){ |
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.
missing space here
|
Some more style nitpickery, but I don't consider those to be objections to getting this in at this stage. Let's get it in unless there are real problems. It can always be fixed later. |
|
Agreed. There are some more style issues (watch out for more than one blank line), but that's not a reason to not merge now. Otherwise, LGTM. |
Bug fixed:
New features (i.e. additions which are not part of already filed bugs)