Cols support - partly solved - first feedback

[svenruppert]

This change is 

[rogozinds]

Reviewed 2 of 18 files at r2. Review status: 2 of 16 files reviewed at latest revision, 8 unresolved discussions.

---

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

    for (Component component : components) {
        addComponent(component);
    }

We don't need it, leave it as it was. Super.addComponents will handle this

---

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

//TODO check if Component is part ow Row Model For whom is this todo?

I think we can check that this row has this component, if yes execute all the code otherwise just ignore

so just wrap everything into if(componenets.contains(c) {

}

---

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

public int usedColAmount() { return getState().usedColAmount(); } That's an API change, but I think this can be usefull, let's keep it for now.

---

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

public RowState getState() { return (RowState) super.getState(); } Why the method visibility has changed?

---

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

Quoted 4 lines of code…

> public void removeColsForComponent(Component component) { > setCols(component, 1); > //markAsDirty(); > }

 

Quoted 4 lines of code…

// public void redraw() { // RowState state = getState(); // // }

Remove this method. It has confusing name, you cant remove cols for component. you can just set it to 1. I think just having setCols(component, 1) is enough

---

integration-tests/pom.xml, line 268 at r2 (raw file):

            </build>
        </profile>
        <!--Docker and SeleniumGrid -->

Why do we need docker? Can you remove it or move it to another PR.

---

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

import com.vaadin.board.Board;
import com.vaadin.board.Row;

I guess this is not needed, as it's the only change.

---

integration-tests/src/test/java/com/vaadin/addon/board/testbenchtests/AbstractParallelTest.java, line 17 at r2 (raw file):

import com.vaadin.testbench.annotations.BrowserConfiguration;
import com.vaadin.testbench.annotations.RunLocally;

Remove runLocally. Basically no tests in production should have this annotation.

---

Comments from Reviewable

[rogozinds]

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

---

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

Previously, rogozinds (rogozinds) wrote…

> public int usedColAmount() { > return getState().usedColAmount(); > } That's an API change, but I think this can be usefull, let's keep it for now.

We discuss this, it kinda hard to come up with a use case for this API. One possible will be, when you want to add items to an existing board/row. Something like this: you have a list of pictures, so you do something like while(images.get) { if(row.usedColAmount>=4) { row = new Row(); } row.addComponent(image); } But it doesn't look like a typical use case, and it can be done smatter. Let's keep the API minimal, because it's much easier to add new API later, than to remove existing one (we need to be compatable).

---

Comments from Reviewable

[svenruppert]

Review status: 2 of 16 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

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

Previously, rogozinds (rogozinds) wrote…

> for (Component component : components) { > addComponent(component); > } We don't need it, leave it as it was. Super.addComponents will handle this

Done.

---

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

Previously, rogozinds (rogozinds) wrote…

> //TODO check if Component is part ow Row Model For whom is this todo? I think we can check that this row has this component, if yes execute all the code otherwise just ignore so just wrap everything into if(componenets.contains(c) { }

Done.

---

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

Previously, rogozinds (rogozinds) wrote…

We discuss this, it kinda hard to come up with a use case for this API. One possible will be, when you want to add items to an existing board/row. Something like this: you have a list of pictures, so you do something like while(images.get) { if(row.usedColAmount>=4) { row = new Row(); } row.addComponent(image); } But it doesn't look like a typical use case, and it can be done smatter. Let's keep the API minimal, because it's much easier to add new API later, than to remove existing one (we need to be compatable).

Done.

---

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

Previously, rogozinds (rogozinds) wrote…

> public RowState getState() { > return (RowState) super.getState(); > } Why the method visibility has changed?

Done.

---

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

Previously, rogozinds (rogozinds) wrote…

> public void removeColsForComponent(Component component) { > setCols(component, 1); > //markAsDirty(); > } >   > // public void redraw() { > // RowState state = getState(); > // > // } Remove this method. It has confusing name, you cant remove cols for component. you can just set it to 1. I think just having setCols(component, 1) is enough

Done.

---

integration-tests/pom.xml, line 268 at r2 (raw file):

Previously, rogozinds (rogozinds) wrote…

Why do we need docker? Can you remove it or move it to another PR.

Done.

---

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

Previously, rogozinds (rogozinds) wrote…

I guess this is not needed, as it's the only change.

Done.

---

integration-tests/src/test/java/com/vaadin/addon/board/testbenchtests/AbstractParallelTest.java, line 17 at r2 (raw file):

Previously, rogozinds (rogozinds) wrote…

Remove runLocally. Basically no tests in production should have this annotation.

Done.

---

Comments from Reviewable

[rogozinds]

Reviewed 2 of 18 files at r2, 2 of 3 files at r3. Review status: 2 of 15 files reviewed at latest revision, 13 unresolved discussions.

---

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

  </modules>

Remove from patch it's not a change

---

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

public void removeCol(Row row, Component component){
    row.removeColsForComponent(component);

  // markAsDirtyRecursive(); if(rows.contains(row)){ } } I think I"ve mentioned it before, that we do not want to have this API. The user can just use row.setCols(Component, int).

Remove this.

---

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

Previously, svenruppert (Sven Ruppert) wrote…

Done.

Remove this method

---

addon/src/main/java/com/vaadin/board/Row.java, line 94 at r3 (raw file):

Quoted 4 lines of code…

> super.removeComponent(c); > //TODO check if Component is part ow Row Model > components.remove(c); > getState(true).cols.remove(c);

so just wrap everything into if(componenets.contains(c) {

}

---

addon/src/main/java/com/vaadin/board/Row.java, line 152 at r3 (raw file):

cols_support

---

integration-tests/src/test/java/com/vaadin/addon/board/testbenchtests/ColRemoveIT.java, line 8 at r4 (raw file):

 *
 */
public class ColRemoveIT extends AbstractParallelTest {

Implement the test.

---

Comments from Reviewable

[rogozinds]

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

---

addon/src/main/java/com/vaadin/board/client/RowConnector.java, line 58 at r4 (raw file):

                .getWidget()
                .getElement()
                .setAttribute("board-cols", strValue);

so here add getWidget().redraw(getWidget().getElement());

---

addon/src/main/java/com/vaadin/board/client/RowWidget.java, line 11 at r4 (raw file):

  public native void redraw()/-{ this.redraw(); }-/; Change this to public native void redraw(Element elem)/-{ elem.redraw(); }-/;

---

Comments from Reviewable

[rogozinds]

Reviewed 1 of 4 files at r5. Review status: 3 of 16 files reviewed at latest revision, 9 unresolved discussions.

---

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

Previously, svenruppert (Sven Ruppert) wrote…

Done.

Still there

---

integration-tests/src/test/java/com/vaadin/addon/board/testbenchtests/AbstractParallelTest.java, line 17 at r2 (raw file):

Previously, svenruppert (Sven Ruppert) wrote…

Done.

Still there.

---

Comments from Reviewable

[svenruppert]

https://github.com/vaadin/board/pull/50