Skip to content

Conversation

@platosha
Copy link
Contributor

Adds signal binding support for the Details component's summary text property through new constructors and binding methods.

Connected to vaadin/flow#23193

🤖 Generated with Claude Code

Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com

platosha and others added 2 commits January 23, 2026 15:59
Adds signal binding support for the Details component's summary text property through new constructors and binding methods.

Connected to vaadin/flow#23193

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@platosha platosha requested a review from DiegoCardoso January 23, 2026 14:01
@sonarqubecloud
Copy link

}

/**
* Binds a signal to update the summary text of the details component.
Copy link
Member

Choose a reason for hiding this comment

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

For consistency public bind* methods should say same message all over. For this case HasText::bindText(Signal<String>) works as a good template. Example is not needed since there are similar examples in most Flow API.

if (getElement().getPropertyRaw("opened") == null) {
setOpened(false);
getElement().setProperty("opened", false);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change needed for the feature to work?

Comment on lines +106 to +108
if (summary == null) {
summary = "";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also done for the other constructor that receives the summary as a String. Does it make sense to move it to the updateSummaryText() method?

if (summary == null || !(summary instanceof Span)) {
updateSummary(new Span(newText));
} else {
summary.getElement().setText(newText);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not covered by any tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants