Skip to content

Conversation

@platosha
Copy link
Contributor

@platosha platosha commented Jan 16, 2026

Connected to: vaadin/flow#23193

Depends on #8507

  • Add new constructors and methods to support binding text to signals
  • Refactor internal text handling using a Text node
  • Improve icon slot management and theme attribute updates

@CLAassistant
Copy link

CLAassistant commented Jan 16, 2026

CLA assistant check
All committers have signed the CLA.

@platosha platosha force-pushed the feat/signals-constructor-bindtext branch from 92f6c71 to 1032ae5 Compare January 16, 2026 17:06
@platosha platosha changed the title feat/signals constructor bindtext feat: add constructors for text binding to Button Jan 16, 2026
@platosha platosha moved this to 🔎Iteration reviews in Vaadin Flow | Hilla | Kits ongoing work Jan 16, 2026
@mshabarov mshabarov self-requested a review January 19, 2026 07:45
Copy link
Contributor

@DiegoCardoso DiegoCardoso left a comment

Choose a reason for hiding this comment

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

Do we need to add integration tests to check the signals behavior? Wouldn't it be enough to add unit tests instead?

@platosha
Copy link
Contributor Author

Do we need to add integration tests to check the signals behavior? Wouldn't it be enough to add unit tests instead?

One reason for ITs on my mind is the dynamic Button behavior regarding the icon. Some aspects are challenging to assert: it's quite possible to make the icon accidentally disappear from the DOM upon a text change, and this error condition would be hard to detect from a unit test context.

Copy link
Contributor

@DiegoCardoso DiegoCardoso left a comment

Choose a reason for hiding this comment

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

IMO this PR could've been split to remove the noise from the MockUI refactor.

Added a couple of comments.

Copy link
Contributor

@mshabarov mshabarov left a comment

Choose a reason for hiding this comment

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

Filed one comment about potential breaking change, but otherwise no comment in addition to these Diego has already mentioned.

@platosha platosha force-pushed the feat/signals-constructor-bindtext branch from f0b04ba to 73b03c5 Compare January 20, 2026 11:40
@platosha
Copy link
Contributor Author

IMO this PR could've been split to remove the noise from the MockUI refactor.

Added a couple of comments.

See #8507

@platosha platosha force-pushed the feat/signals-constructor-bindtext branch from 73b03c5 to c439e17 Compare January 20, 2026 11:47
Connected to: vaadin/flow#23193

- Add new constructors and methods to support binding text to signals
- Refactor internal text handling using a Text node
- Improve icon slot management and theme attribute updates
- Use single `SignalPropertySupport` instance for managing signal-originating button text changes
- Replace direct DOM updates with centralized handler method
- Maintain consistency between setText and bindText operations
@platosha platosha force-pushed the feat/signals-constructor-bindtext branch from adea1db to cbfacd4 Compare January 20, 2026 14:00
@sonarqubecloud
Copy link

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

Labels

Projects

Status: 🔎Iteration reviews

Development

Successfully merging this pull request may close these issues.

5 participants