vaadin / board

Framework 8 version of vaadin-board
Other
8 stars 10 forks source link

Feature/dash 27 one commit #57

Closed svenruppert closed 7 years ago

svenruppert commented 7 years ago

This change is Reviewable

rogozinds commented 7 years ago

Reviewed 25 of 53 files at r2. Review status: 25 of 50 files reviewed at latest revision, 3 unresolved discussions.


integration-tests/src/main/java/com/vaadin/addon/board/testUI/CompatBasicChartUI.java, line 20 at r2 (raw file):

protected abstract Supplier nextChartInstance(); NO need to have this , you can just use getChart() when adding to layout.


integration-tests/src/main/java/com/vaadin/addon/board/testUI/CompatBasicChartUI.java, line 26 at r2 (raw file):

.apply(Stream.of( As far as I know streams are parallel, so there is no order there. So if you then want to have different components to add, there order will be random. I don't know is it a problem right now, but just a potential bug, that hard to catch.


integration-tests/src/main/java/com/vaadin/addon/board/testUI/CompatBasicComponents.java, line 41 at r2 (raw file):

protected Supplier<Class<? extends Component>> nextClass() {
  return () -> Button.class;
}

You don't need Supplier here, you can just have a method that returns a Component out of it. Same for all below. You can a method that returns a tested component (Layout with all the stuff included) or a collection of components you add to your layout in a parent class. I think it will be much clearer.


Comments from Reviewable

svenruppert commented 7 years ago

Reviewed 3 of 53 files at r2. Review status: 25 of 50 files reviewed at latest revision, 3 unresolved discussions.


integration-tests/src/main/java/com/vaadin/addon/board/testUI/CompatBasicChartUI.java, line 26 at r2 (raw file):

Previously, rogozinds (rogozinds) wrote…
> .apply(Stream.of( As far as I know streams are parallel, so there is no order there. So if you then want to have different components to add, there order will be random. I don't know is it a problem right now, but just a potential bug, that hard to catch.

No, Streams are not parallel. only if you are switching them. So the order is fixed..


Comments from Reviewable

rogozinds commented 7 years ago

Review status: 25 of 50 files reviewed at latest revision, 3 unresolved discussions.


integration-tests/src/main/java/com/vaadin/addon/board/testUI/CompatBasicChartUI.java, line 20 at r2 (raw file):

Previously, rogozinds (rogozinds) wrote…
> protected abstract Supplier nextChartInstance(); NO need to have this , you can just use getChart() when adding to layout.

What about his one can you change that ?


Comments from Reviewable

rogozinds commented 7 years ago

Reviewed 24 of 53 files at r2. Review status: 49 of 50 files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

rogozinds commented 7 years ago

Reviewed 1 of 53 files at r2. Review status: all files reviewed at latest revision, 4 unresolved discussions.


addon/src/main/java/com/vaadin/board/Board.java, line 34 at r2 (raw file):

  • Creates a new row and adds the given components to the row.
  • hing is change in this file

Comments from Reviewable

svenruppert commented 7 years ago

Reviewed 1 of 2 files at r3. Review status: 49 of 50 files reviewed at latest revision, 4 unresolved discussions.


addon/src/main/java/com/vaadin/board/Board.java, line 34 at r2 (raw file):

Previously, rogozinds (rogozinds) wrote…
> * Creates a new row and adds the given components to the row. > *

Nothing is change in this file

Done.


integration-tests/src/main/java/com/vaadin/addon/board/testUI/CompatBasicChartUI.java, line 20 at r2 (raw file):

Previously, rogozinds (rogozinds) wrote…
What about his one can you change that ?

Done.


integration-tests/src/main/java/com/vaadin/addon/board/testUI/CompatBasicChartUI.java, line 26 at r2 (raw file):

Previously, svenruppert (Sven Ruppert) wrote…
No, Streams are not parallel. only if you are switching them. So the order is fixed..

Done.


Comments from Reviewable

svenruppert commented 7 years ago

Review status: 49 of 50 files reviewed at latest revision, 3 unresolved discussions.


integration-tests/src/main/java/com/vaadin/addon/board/testUI/CompatBasicComponents.java, line 41 at r2 (raw file):

Previously, rogozinds (rogozinds) wrote…
> protected Supplier> nextClass() { > return () -> Button.class; > } You don't need Supplier here, you can just have a method that returns a Component out of it. Same for all below. You can a method that returns a tested component (Layout with all the stuff included) or a collection of components you add to your layout in a parent class. I think it will be much clearer.

as mentioned, I will remove it


Comments from Reviewable

svenruppert commented 7 years ago

Reviewed 1 of 2 files at r3. Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

svenruppert commented 7 years ago

Reviewed 3 of 6 files at r1, 3 of 53 files at r2, 1 of 2 files at r4. Review status: 49 of 50 files reviewed at latest revision, 3 unresolved discussions.


integration-tests/src/main/java/com/vaadin/addon/board/testUI/CompatBasicComponents.java, line 41 at r2 (raw file):

Previously, svenruppert (Sven Ruppert) wrote…
as mentioned, I will remove it

Done.


Comments from Reviewable

svenruppert commented 7 years ago

Reviewed 1 of 2 files at r4. Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

rogozinds commented 7 years ago
:lgtm:

Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable