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

Add Grid#setItemDetailsRenderer #2772

Merged
merged 9 commits into from
Oct 27, 2017

Conversation

ahie
Copy link
Contributor

@ahie ahie commented Oct 25, 2017

This change is Reviewable

@gilberto-torrezan
Copy link
Contributor

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.


flow-components-parent/flow-components/src/main/java/com/vaadin/ui/grid/Grid.java, line 802 at r1 (raw file):

    public void setItemDetailsRenderer(TemplateRenderer<T> renderer) {
        if (detailsTemplate != null) {
            getElement().getNode().runWhenAttached(

Why do you need to remove the current template only when attached?


flow-components-parent/flow-components/src/main/java/com/vaadin/ui/grid/Grid.java, line 813 at r1 (raw file):

        detailsTemplate = newDetailsTemplate;
        getElement().getNode().runWhenAttached(

Same question here, but related to append.


Comments from Reviewable

@ahie
Copy link
Contributor Author

ahie commented Oct 26, 2017

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


flow-components-parent/flow-components/src/main/java/com/vaadin/ui/grid/Grid.java, line 802 at r1 (raw file):

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

Why do you need to remove the current template only when attached?

I guess it is not really needed. Just wanted to reduce the possibility of undefined behavior.


Comments from Reviewable

@ahie
Copy link
Contributor Author

ahie commented Oct 26, 2017

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


flow-components-parent/flow-components/src/main/java/com/vaadin/ui/grid/Grid.java, line 813 at r1 (raw file):

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

Same question here, but related to append.

Removed both runWhenAttached.


Comments from Reviewable

@gilberto-torrezan
Copy link
Contributor

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@ahie ahie force-pushed the 2721-grid-row-details-template branch from a9fe1f5 to aeb9e40 Compare October 26, 2017 12:04
@gilberto-torrezan
Copy link
Contributor

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 2 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 Grid.java#L334: Define a constant instead of duplicating this literal "template" 3 times. rule
  2. CRITICAL Grid.java#L336: Define a constant instead of duplicating this literal "innerHTML" 3 times. rule

@gilberto-torrezan gilberto-torrezan merged commit 176a695 into master Oct 27, 2017
@gilberto-torrezan gilberto-torrezan deleted the 2721-grid-row-details-template branch October 27, 2017 09:53
@SomeoneToIgnore SomeoneToIgnore added this to the 1.0.0.alpha8 milestone Nov 2, 2017
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.

None yet

4 participants