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

fix: do not set placeholder to empty string by default #7573

Merged
merged 5 commits into from
Jul 25, 2024

Conversation

web-padawan
Copy link
Member

Description

Fixes #7551

Changed to align with InputControlMixin by removing default empty string value, so that we don't have placeholder attribute set by default. Also fixed the logic for restoring placeholder property in selected items observer.

Note, the property has reflectToAttribute so technically setting empty string can still cause that to happen (which is the case for all input fields). I don't think we can easily fix that without a breaking change.

Type of change

  • Bugfix

@web-padawan web-padawan requested a review from vursen July 19, 2024 08:18
@vursen
Copy link
Contributor

vursen commented Jul 22, 2024

Looks like the change results in a regression: after deselecting all items, the original placeholder doesn't come back, and instead, the placeholder shows the last deselected item.

Screen.Recording.2024-07-22.at.12.27.53.mov
<vaadin-multi-select-combo-box placeholder="Placeholder"></vaadin-multi-select-combo-box>

<script>
  const comboBox = document.querySelector('vaadin-multi-select-combo-box');
  comboBox.items = ['Item 1'];
  comboBox.selectedItems = ['Item 1'];
</script>

@web-padawan
Copy link
Member Author

Looks like the change results in a regression: after deselecting all items, the original placeholder doesn't come back

Thanks, this actually is related to the fact that in your example (and the dev page) we use <script> without type set to module (and therefore both placeholder and selectedItems are set before the component gets upgraded.

Added a fix for this case and updated tests to reproduce it (by replacing fixtureSync() usage with manual adding to DOM).

@vursen
Copy link
Contributor

vursen commented Jul 22, 2024

Added a fix for this case and updated tests to reproduce it

Looks like this didn't fix the regression completely. When deselecting all items without an initial placeholder, the placeholder doesn't clear and shows the last deselected item again.

<vaadin-multi-select-combo-box></vaadin-multi-select-combo-box>

<script>
  const comboBox = document.querySelector('vaadin-multi-select-combo-box');
  comboBox.items = ['Item 1'];
  comboBox.selectedItems = ['Item 1'];
</script>

@web-padawan
Copy link
Member Author

When deselecting all items without an initial placeholder, the placeholder doesn't clear

Thanks, fixed and added a test with setting placeholder to undefined.

Copy link

sonarcloud bot commented Jul 24, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@web-padawan web-padawan merged commit 4d80c5c into main Jul 25, 2024
9 checks passed
@web-padawan web-padawan deleted the fix/mscb-placeholder-empty branch July 25, 2024 06:37
web-padawan added a commit that referenced this pull request Jul 25, 2024
Co-authored-by: Serhii Kulykov <iamkulykov@gmail.com>
web-padawan added a commit that referenced this pull request Jul 25, 2024
Co-authored-by: Serhii Kulykov <iamkulykov@gmail.com>
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.5.0.alpha6 and is also targeting the upcoming stable 24.5.0 version.

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

Successfully merging this pull request may close these issues.

Multi-select-combo-box placeholder attribute active when no placeholder text set
3 participants