Skip to content
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

Feature/67 startup screen #91

Open
wants to merge 9 commits into
base: development
Choose a base branch
from

Conversation

romansavelyev
Copy link

@romansavelyev romansavelyev commented Aug 4, 2016

This change is Reviewable

@Guitarheroua
Copy link
Member

Reviewed 2 of 20 files at r1.
Review status: 2 of 20 files reviewed at latest revision, 1 unresolved discussion.


pllug-presentation-system/qpm .exe, line 0 [r1] ([raw file](https://github.com/pllug/pllug-presentation-system-2/blob/34c27fe2196ffd7a949da88d3f84a8c94ced6793/pllug-presentation-system/qpm .exe#L0)):
Please, remove qpm.exe from PPS 2 Repository.


Comments from Reviewable

@Guitarheroua
Copy link
Member

Review status: 2 of 20 files reviewed at latest revision, 2 unresolved discussions.


doc/workflow/qpm_packages_installation.md, line 5 [r1] (raw file):

 * Download [qpm package manager](https://www.qpm.io/). 
 * Put qpm.exe in PPS project folder.
 * Use `qpm install` in command line.

Please, mention, that we need to cd to/PPS2/folder in command line, before run qpm install.


Comments from Reviewable

@Guitarheroua
Copy link
Member

Reviewed 2 of 20 files at r1.
Review status: 4 of 20 files reviewed at latest revision, 3 unresolved discussions.


pllug-presentation-system/resources/qml/main.qml, line 16 [r1] (raw file):

    title: "Qt Quick Controls 2"

    Rectangle {

Please, create separate element for Startup window.


Comments from Reviewable

@Guitarheroua
Copy link
Member

Review status: 4 of 20 files reviewed at latest revision, 4 unresolved discussions.


pllug-presentation-system/resources/qml/main.qml, line 14 [r1] (raw file):

    height: 480
    visible: true
    title: "Qt Quick Controls 2"

Give appropriate name for window PLLUG Presentation System 2.0.


Comments from Reviewable

@Guitarheroua
Copy link
Member

Review status: 1 of 20 files reviewed at latest revision, 5 unresolved discussions.


pllug-presentation-system/pllug-presentation-system.pro, line 22 [r2] (raw file):

DISTFILES += \
    resources/qml/qml files/StartupScreen.qml

Please, remove this, as you have your StartupScreen.qml in resource file.


Comments from Reviewable

@Guitarheroua
Copy link
Member

Reviewed 2 of 6 files at r2.
Review status: 3 of 20 files reviewed at latest revision, 6 unresolved discussions.


pllug-presentation-system/resources/qml/qml files/StartupScreen.qml, line 7 [r2] ([raw file](https://github.com/pllug/pllug-presentation-system-2/blob/89218a2c8a6bce086bc59915bb2e0d385c28ae94/pllug-presentation-system/resources/qml/qml files/StartupScreen.qml#L7)):

Rectangle {
    id: elements
    width: window.width;

Please, move setting of height and width to main.qml, where you create StartupScreen element.


Comments from Reviewable

@Guitarheroua
Copy link
Member

Review status: 3 of 20 files reviewed at latest revision, 7 unresolved discussions.


pllug-presentation-system/resources/qml/qml files/StartupScreen.qml, line 10 [r2] ([raw file](https://github.com/pllug/pllug-presentation-system-2/blob/89218a2c8a6bce086bc59915bb2e0d385c28ae94/pllug-presentation-system/resources/qml/qml files/StartupScreen.qml#L10)):

    height: window.height

    FocusScope {

Why you wrap FocusScope into Rectangle? Why do not use only FocuseScope ?


Comments from Reviewable

@Guitarheroua
Copy link
Member

Review status: 3 of 20 files reviewed at latest revision, 8 unresolved discussions.


pllug-presentation-system/resources/qml/qml files/StartupScreen.qml, line 12 [r2] ([raw file](https://github.com/pllug/pllug-presentation-system-2/blob/89218a2c8a6bce086bc59915bb2e0d385c28ae94/pllug-presentation-system/resources/qml/qml files/StartupScreen.qml#L12)):

    FocusScope {
        id: mainView
        width: parent.width; height: parent.height

If you know, that your child item will inherit sizes from his parent, use anchor.fill = parent instead of width: parent.width; height: parent.height


Comments from Reviewable

@Guitarheroua
Copy link
Member

Review status: 3 of 20 files reviewed at latest revision, 9 unresolved discussions.


pllug-presentation-system/resources/qml/qml files/StartupScreen.qml, line 10 [r2] ([raw file](https://github.com/pllug/pllug-presentation-system-2/blob/89218a2c8a6bce086bc59915bb2e0d385c28ae94/pllug-presentation-system/resources/qml/qml files/StartupScreen.qml#L10)):

Previously, Guitarheroua (Andrii) wrote…

Why you wrap FocusScope into Rectangle? Why do not use only FocuseScope ?

Why you use FocuseScope here?

pllug-presentation-system/resources/qml/qml files/StartupScreen.qml, line 19 [r2] ([raw file](https://github.com/pllug/pllug-presentation-system-2/blob/89218a2c8a6bce086bc59915bb2e0d385c28ae94/pllug-presentation-system/resources/qml/qml files/StartupScreen.qml#L19)):

            anchors.fill: parent
            width: parent.width;
            height:parent.height

Please, remove


Comments from Reviewable

@Guitarheroua
Copy link
Member

Review status: 3 of 20 files reviewed at latest revision, 9 unresolved discussions.


pllug-presentation-system/resources/qml/qml files/StartupScreen.qml, line 9 [r2] ([raw file](https://github.com/pllug/pllug-presentation-system-2/blob/89218a2c8a6bce086bc59915bb2e0d385c28ae94/pllug-presentation-system/resources/qml/qml files/StartupScreen.qml#L9)):

    width: window.width;
    height: window.height

Please, remove width and height here.


Comments from Reviewable

@Guitarheroua
Copy link
Member

Review status: 3 of 20 files reviewed at latest revision, 9 unresolved discussions.


pllug-presentation-system/vendor/vendor.pri, line 1 [r2] (raw file):

Please, remove folder vendor from repository.


Comments from Reviewable

@Guitarheroua
Copy link
Member

Review status: 3 of 20 files reviewed at latest revision, 9 unresolved discussions.


pllug-presentation-system/resources/qml/qml files/StartupScreen.qml, line 19 [r2] ([raw file](https://github.com/pllug/pllug-presentation-system-2/blob/89218a2c8a6bce086bc59915bb2e0d385c28ae94/pllug-presentation-system/resources/qml/qml files/StartupScreen.qml#L19)):

Previously, Guitarheroua (Andrii) wrote…

Please, remove

height and width.

Comments from Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants