-
Notifications
You must be signed in to change notification settings - Fork 5
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
In SelectMulti (& confirm in SelectSingle), determine currently selected options by *value*, rather than by *reference* #78
Labels
Comments
jackkoppa
added a commit
that referenced
this issue
Feb 21, 2021
Add specs (including two that currently fail, because of the bug/feature request in #78) for confirming that, when the list of `options` is updated in the parent component, we are still able to accurately compare our current `selectedOptions` array to the new `options` list, to determine which should be displayed as selected. Next steps are to actually fix the code in SelectMulti so the specs can pass Additional work: refactoring the specs file so that there can be a bit of easily-repeatable specs, in different component initialization setups (primary example so far is that we can extract the 'mount' method, so each type of spec only has to define the new SelectMulti template string it would like to use, and grouping all the specs related to confirming behavior when selecting options, **and** passing in a new reference to the list of options) Note: we've still tried to follow https://github.com/mawrkus/js-unit-testing-guide as much as possible Namely, while there is some repeated code extracted to functions, we are still NOT: * involving logic in our specs - we are NOT using if/else or loops; only using callbacks when we need more than simple extraction of repeated code * using comments to document the specs - our `describe` and `it` blocks are fairly verbose, because we are preferring long spec descriptions, to requiring devs to read code comments in order to understand the purpose of each spec
jackkoppa
added a commit
that referenced
this issue
Feb 22, 2021
Like the specs added to SelectMulti, these specs will check whether SelectSingle is able to compare the `options` array to the list of currently selected options using **value**, rather than **reference** Additional work: refactor SelectSingle specs to match extracted-shared-code approach of SelectMulti approach TODO: 2 specs for SelectSingle are currently failing, and this seems to indicate a possible bug with the handling of `uniqueIdField` in SelectSingle - this needs to be fixed. Yay specs!
jackkoppa
added a commit
that referenced
this issue
Feb 22, 2021
In SelectMulti, use a new helper method - optionExistsByUniqueId - to compare options in the `options` array, to those in the `selectedOptions` array, **by value**, rather than by reference We do this by saying "uniqueIdField is going to be the property on each SelectOption, guaranteed to be the identifier for that option. So, in order to find if an option is 'selected', we'll reference that property on each option" This allows us to move outside the realm of "just check the reference to each object", and solves the maintenance/complex use case problems encountered by the Politico team & described in #78
jackkoppa
added a commit
that referenced
this issue
Feb 22, 2021
When refactoring specs for SelectSingle to better match the improved specs in SelectMulti, esp. re: #78 (compare by value not by reference), we caught a bug for #76 re: how SelectSingle highlights the currently selected option in the dropdown This is now fixed by referencing `uniqueIdField` when setting `selectedIndex`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Currently,
SelectMulti
uses a simpleindexOf
to determine if a given option is currently selected.It does this by checking "is this item from the
options
array, present in the currentselectedOptions
?"See here: https://github.com/politico/vue-accessible-selects/blob/v0.12.0/src/SelectMulti.vue#L272
Or here: https://github.com/politico/vue-accessible-selects/blob/v0.12.0/src/SelectMulti.vue#L356
This works fine for most cases: we expect that the
options
passed into the component will remain the same as when the component was initialized, and as such, it should be safe to compare by reference, asindexOf
does, to see "is this specific object reference in the selected array"A problem occurs, however, as soon as we start to have more complicated parent components, that might update the list of
options
passed intoSelectMulti
- perhaps based on an API call. Now, even though the original values of theoptions
list may still exist, they now have a new reference. And thus,indexOf
is no longer an accurate way to find these options & determine if they are in theselectedOptions
arraySolution
Allow for "find by value", rather than "find by reference". i.e. let's choose a way to reliably compare the items in the
options
array, with those that have been added to theselectedOptions
array. That does not rely on objects in each array having the same reference. Since the reference approach won't be terribly maintainable, and will quickly lead to the types of confusing bugs we've already begun to experience @ Politico.In this issue, then, let's first write a failing test:
"When I update the
options
prop to be an array full of new references, selected options are still correctly displayed as long as they match the value of currentselectedOptions
"Then, let's fix the code so the spec passes
Note:
SelectSingle
is already doing compare-by-value here, but let's make sure it has the appropriate specs & testing regardlessThe text was updated successfully, but these errors were encountered: