Skip to content
This repository has been archived by the owner on Oct 8, 2020. It is now read-only.

Group refresh event #88

Merged
merged 4 commits into from
Jan 29, 2019
Merged

Group refresh event #88

merged 4 commits into from
Jan 29, 2019

Conversation

campbellbartlett
Copy link
Contributor

@campbellbartlett campbellbartlett commented Jan 24, 2019 •
edited by ZheSun88

This change is Reviewable

Campbell Bartlett added 2 commits January 25, 2019 08:37
CBG now only updates a single item if refreshItem(item) is called
1179943 
Fix for vaadin#85:
Calling dataProvider.refreshItem(item) will reset checkboxgroup.

It should instead check for the type of the event fired by data provide, and if it is of type DataChangeEvent.DataRefreshEvent then update only the necessary item.
Undo Intelij autoformatting
de9b0aa
@CLAassistant
Copy link

CLAassistant commented Jan 24, 2019 •
edited

CLA assistant check
All committers have signed the CLA.

@campbellbartlett campbellbartlett mentioned this pull request Jan 24, 2019
Closed
@pleku pleku self-requested a review January 25, 2019 09:05
@pleku pleku self-assigned this Jan 25, 2019
pleku
pleku suggested changes Jan 25, 2019
View reviewed changes
Copy link
Contributor

@pleku pleku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello and thanks for the contribution! It is just missing two method calls and the test case for that specific scenario (see comments).

src/main/java/com/vaadin/flow/component/checkbox/CheckboxGroup.java Outdated Show resolved Hide resolved
src/main/java/com/vaadin/flow/component/checkbox/CheckboxGroup.java Outdated Show resolved Hide resolved
src/test/java/com/vaadin/flow/component/checkbox/tests/CheckboxGroupTest.java Outdated Show resolved Hide resolved
@pleku
Copy link
Contributor

pleku commented Jan 25, 2019

@campbellbartlett if you don't have time to contribute the changes before Monday, we can do those instead. We are refactoring the project structure and want to merge this PR before land those changes since otherwise there will be merge conflicts.

@campbellbartlett
Copy link
Contributor Author

Thanks for the review!
That is a good point about the ids, I should have considered that.
I am away this weekend as it’s a holiday weekend in Australia, but I’m happy to push the changes first thing on Monday (Sydney time) if that’s not too late.

@pleku
Copy link
Contributor

pleku commented Jan 28, 2019

We had to merge the other PR with the refactoring, so this PR has now conflicts. Sorry about that.

pleku
pleku approved these changes Jan 28, 2019
View reviewed changes
Copy link
Contributor

@pleku pleku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 but changes need to be applied to a new location due to the refactoring of the project to the multiple projects.

pleku
pleku reviewed Jan 28, 2019
View reviewed changes
Copy link
Contributor

@pleku pleku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pleku)

@vaadin-bot
Copy link

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 CheckboxGroup.java#L78: Make "item" transient or serializable. rule
  2. CRITICAL CheckboxGroup.java#L145: Remove usage of generic wildcard type. rule

pleku
pleku approved these changes Jan 29, 2019
View reviewed changes
Copy link
Contributor

@pleku pleku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@pleku pleku merged commit 8059bbd into vaadin:master Jan 29, 2019
@pleku pleku added this to the 1.3.0.beta1 milestone Jan 29, 2019
ujoni pushed a commit that referenced this pull request Feb 8, 2019
@campbellbartlett
Group refresh event (#88)
802a5b6 
Fixes #85
Calling dataProvider.refreshItem(item) will reset checkboxgroup.

It should instead check for the type of the event fired by data provide, and if it is of type DataChangeEvent.DataRefreshEvent then update only the necessary item.
@ujoni ujoni mentioned this pull request Feb 8, 2019
Merged
denis-anisimov pushed a commit that referenced this pull request Feb 11, 2019
@ujoni
Group refresh event (#88) (#90)
128f3ac 
Fixes #85
Calling dataProvider.refreshItem(item) will reset checkboxgroup.

It should instead check for the type of the event fired by data provide, and if it is of type DataChangeEvent.DataRefreshEvent then update only the necessary item.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pleku pleku pleku approved these changes

Assignees

@pleku pleku

Labels
None yet
Projects
None yet
Milestone
1.3.0.beta1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@campbellbartlett @CLAassistant @pleku @vaadin-bot