Fixed syncSelectValue() for data-display#100
Merged
Tsjippy merged 3 commits intobluzky:masterfrom Aug 27, 2025
Merged
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates syncSelectValue to perform case-insensitive matching of dropdown options based on the data-display attribute (falling back to textContent) with an additional fallback to value, and renames the loop variable for clarity. Class diagram for updated syncSelectValue logic in NiceSelectclassDiagram
class NiceSelect {
options: Array
syncSelectValue()
}
class Option {
dataset.display
textContent
value
selected
}
NiceSelect --> Option : uses in syncSelectValue
Flow diagram for option matching in syncSelectValueflowchart TD
A[Start: For each item in options] --> B{Find option by data-display or textContent}
B -- Found --> D[Set selected state]
B -- Not Found --> C{Find option by value}
C -- Found --> D[Set selected state]
C -- Not Found --> E[Continue to next item]
D --> F[Next item]
E --> F[Next item]
F --> G[End]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/js/nice-select2.js:442` </location>
<code_context>
this.options.forEach(item =>{
- let option = Array.from(select.options).find(option => option.textContent === item.data.text);
+ let matchingOption = Array.from(select.options).find(option => {
+ return (option.dataset.display || option.textContent) === item.data.text;
+ });
+
</code_context>
<issue_to_address>
Consider normalizing text comparison for robustness.
Direct string comparison may fail if there are differences in whitespace or case. Normalizing both values before comparison will improve reliability, especially when handling user input or external data.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/js/nice-select2.js:456` </location>
<code_context>
- if(option == undefined){
- option = Array.from(select.options).find(option => option.textContent === item.data.value);
+ if (matchingOption == undefined) {
+ // no matching option was found, continue
+ return;
}
</code_context>
<issue_to_address>
Silently skipping unmatched options may hide data issues.
Consider adding a log or warning when no matching option is found to help identify data mismatches during debugging.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (matchingOption == undefined) {
// no matching option was found, continue
return;
=======
if (matchingOption == undefined) {
console.warn(`No matching option found for value: "${item.data.value}" in select element`, select);
return;
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/js/nice-select2.js:462` </location>
<code_context>
if(item.attributes.selected){
- option.selected = true;
+ matchingOption.selected = true;
} else {
- option.selected = false;
+ matchingOption.selected = false;
}
});
</code_context>
<issue_to_address>
Directly setting selected property may not trigger change events.
If your application relies on change events, dispatch one manually after updating the selection.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if(item.attributes.selected){
matchingOption.selected = true;
} else {
matchingOption.selected = false;
}
=======
if(item.attributes.selected){
matchingOption.selected = true;
} else {
matchingOption.selected = false;
}
// Manually dispatch change event after updating selection
const event = new Event('change', { bubbles: true });
select.dispatchEvent(event);
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The changes in this PR address issue #99 by allowing
syncSelectValue()to takedata-displayinto account when searching for an option matching each item in the dropdown list.This solution is fully tested in a local environment and confirmed to work regardless of
data-displaybeing used or not.The name
matchingOptionwas used for the variable in the outer scope, so that there is no confusion with theoptionvariable used inside thefindcallback functions. While the previous naming doesn't create any functional problem, the new one makes the code easier to read.Summary by Sourcery
Fix syncSelectValue to account for the data-display attribute when matching dropdown options and improve variable naming for clarity.
Bug Fixes:
Enhancements: