vaadin / patient-portal-demo-flow

0 stars 0 forks source link

Lazy-loading component grid #67

Closed elmot closed 6 years ago

elmot commented 6 years ago

Fixes #61


This change is Reviewable

pekam commented 6 years ago

Review status: 0 of 11 files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.


src/main/java/com/vaadin/flow/demo/patientportal/ui/PatientsView.java, line 76 at r2 (raw file):

        grid.addColumn(fullNameProvider).setHeader("Name")
                .setComparator(fullNameProvider).setFlexGrow(1)
                .addClassName("strong");

I don't think adding a class name to a column has any effect. Can you verify?


src/main/java/com/vaadin/flow/demo/patientportal/ui/PatientsView.java, line 98 at r2 (raw file):

    public interface PatientsViewModel extends TemplateModel {

        String getCurrentPatientId();

Do we even need this anymore? Could we just remove the entire interface at this point?


src/main/java/com/vaadin/flow/demo/patientportal/ui/patients/PatientDetails.java, line 35 at r2 (raw file):

@Tag("patient-details")
@HtmlImport("frontend://components/main/patients/patient-details.html")
@RoutePrefix("patients")

Why did you change the route? I don't think it's a big deal what the route is but maybe changing it at this point may cause that we need to change something in the scalability tests? Maybe, I don't really know.


src/main/java/com/vaadin/flow/demo/patientportal/ui/patients/PatientJournal.java, line 73 at r2 (raw file):

        grid.setItemDetailsRenderer(TemplateRenderer.of(
                "<section class=\"details\"><h3>Notes</h3><article>[[entry]]</article></section>"));
        grid.setDetailsVisibleOnClick(true);

This should be true by default.


src/main/webapp/frontend/components/main/patients/patient-journal.html, line 71 at r2 (raw file):


    <vaadin-grid id="journalGrid"/>
<!--todo restore narrow functionality

I don't understand what this comment means. And why do you keep the old grid here commented out?


Comments from Reviewable

SomeoneToIgnore commented 6 years ago

pom.xml, line 72 at r2 (raw file):

                <scope>import</scope>
            </dependency>
            <dependency><!--TODO remove when nexp platform version is out-->

Since it's Monday, next version should be available, I propose to remove those TODOs


Comments from Reviewable

elmot commented 6 years ago

Review status: 0 of 15 files reviewed at latest revision, 6 unresolved discussions.


pom.xml, line 72 at r2 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…
Since it's Monday, next version should be available, I propose to remove those TODOs

Done.


src/main/java/com/vaadin/flow/demo/patientportal/ui/PatientsView.java, line 76 at r2 (raw file):

Previously, pekam (Pekka Maanpää) wrote…
I don't think adding a class name to a column has any effect. Can you verify?

You are right. But why the method (and the whole interface) is here?

Rewritten


src/main/java/com/vaadin/flow/demo/patientportal/ui/PatientsView.java, line 98 at r2 (raw file):

Previously, pekam (Pekka Maanpää) wrote…
Do we even need this anymore? Could we just remove the entire interface at this point?

Removed


src/main/java/com/vaadin/flow/demo/patientportal/ui/patients/PatientDetails.java, line 35 at r2 (raw file):

Previously, pekam (Pekka Maanpää) wrote…
Why did you change the route? I don't think it's a big deal what the route is but maybe changing it at this point may cause that we need to change something in the scalability tests? Maybe, I don't really know.

Because old routes won't work. As it was already discussed, paths are coverted /patients/12/journal => /patients/journal/12


src/main/java/com/vaadin/flow/demo/patientportal/ui/patients/PatientJournal.java, line 73 at r2 (raw file):

Previously, pekam (Pekka Maanpää) wrote…
This should be `true` by default.

Looks like row details does not work at all when html template is used. Workaround is made, elements grid issue tbd


src/main/webapp/frontend/components/main/patients/patient-journal.html, line 71 at r2 (raw file):

Previously, pekam (Pekka Maanpää) wrote…
I don't understand what this comment means. And why do you keep the old grid here commented out?

More explanations added


Comments from Reviewable

pekam commented 6 years ago

Reviewed 2 of 11 files at r1, 1 of 1 files at r2, 10 of 12 files at r3, 2 of 2 files at r4. Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks broke.


src/main/java/com/vaadin/flow/demo/patientportal/ui/PatientsView.java, line 76 at r2 (raw file):

Previously, elmot (Ilia Motornyi) wrote…
You are right. But why the method (and the whole interface) is here? Rewritten

Good question. I created a ticket https://github.com/vaadin/vaadin-grid-flow/issues/65


src/main/java/com/vaadin/flow/demo/patientportal/ui/patients/PatientEditor.java, line 129 at r4 (raw file):

        patientService.deletePatient(getPatient());
        getUI().ifPresent(ui -> ui.navigateTo("patients"));
        close();

Close is just navigating to the patient profile we just removed.


src/main/webapp/frontend/components/main/main-view.html, line 64 at r4 (raw file):


      <a id="logout" class="right" on-click="logout">
        <iron-icon icon="vaadin:exit-o"></iron-icon>

Shouldn't you import vaadin-icons for this to work? Or is it inside my-icons.html?


src/main/webapp/frontend/components/main/patients/patient-details.html, line 71 at r4 (raw file):

    <nav class="details-nav">
      <a router-link href="patients">
        <iron-icon icon="vaadin:arrow-long-left"></iron-icon>

is vaadin-icons imported


src/main/webapp/frontend/components/main/patients/patient-journal.html, line 71 at r2 (raw file):

Previously, elmot (Ilia Motornyi) wrote…
More explanations added

Maybe would be better to explain in the ticket since there is one and just have short explanation with the link here.


src/main/webapp/frontend/components/main/patients/patients-view.html, line 36 at r4 (raw file):

        }
      }
      .strong {

Did you use this elsewhere than in the column? If not, this can be removed.


src/main/webapp/frontend/components/main/patients/patients-view.html, line 43 at r4 (raw file):

    <iron-media-query query="(max-width: 600px)" query-matches="{{narrow}}"></iron-media-query>
    <vaadin-grid id="patientsGrid">
<!-- todo restore narrow functionality

Same as above


Comments from Reviewable

elmot commented 6 years ago

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks broke.


src/main/java/com/vaadin/flow/demo/patientportal/ui/patients/PatientEditor.java, line 129 at r4 (raw file):

Previously, pekam (Pekka Maanpää) wrote…
Close is just navigating to the patient profile we just removed.

Done.


src/main/webapp/frontend/components/main/main-view.html, line 64 at r4 (raw file):

Previously, pekam (Pekka Maanpää) wrote…
Shouldn't you import `vaadin-icons` for this to work? Or is it inside `my-icons.html`?

imported in parent layout


src/main/webapp/frontend/components/main/patients/patient-details.html, line 71 at r4 (raw file):

Previously, pekam (Pekka Maanpää) wrote…
is `vaadin-icons` imported

It is imported in parent layout, patients-view.


src/main/webapp/frontend/components/main/patients/patient-journal.html, line 71 at r2 (raw file):

Previously, pekam (Pekka Maanpää) wrote…
Maybe would be better to explain in the ticket since there is one and just have short explanation with the link here.

Done.


src/main/webapp/frontend/components/main/patients/patients-view.html, line 36 at r4 (raw file):

Previously, pekam (Pekka Maanpää) wrote…
Did you use this elsewhere than in the column? If not, this can be removed.

Done


src/main/webapp/frontend/components/main/patients/patients-view.html, line 43 at r4 (raw file):

Previously, pekam (Pekka Maanpää) wrote…
Same as above

Done.


Comments from Reviewable

vaadin-bot commented 6 years ago

SonarQube analysis reported 9 issues

Watch the comments in this conversation to review them.

4 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. CRITICAL AbstractPatientTemplate.java#L56: Annotate the "PatientTemplateModel" interface with the @FunctionalInterface annotation rule
  2. MINOR AbstractPatientTemplate.java#L19: Remove this unused import 'com.vaadin.demo.entities.JournalEntry'. rule
  3. MINOR AbstractPatientTemplate.java#L23: Remove this unused import 'com.vaadin.flow.demo.patientportal.converters.AppointmentTypeToStringConverter'. rule
  4. MINOR AbstractPatientTemplate.java#L36: Remove this unused import 'java.util.List'. rule
pekam commented 6 years ago

Reviewed 3 of 3 files at r5. Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


Comments from Reviewable