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

Change multiselect cache structure #2412

Merged
merged 1 commit into from
Sep 28, 2015

Conversation

meghaarora42
Copy link
Contributor

The existing structure used an Object as a cache where the keys are the
values of option elements, and values are the HTML content of the option
elements. In Javascript, if a numeric value in the form of a string is
assigned as a key, it gets converted to an integer. Optimization
routines would then order the object to ensure faster access to
elements. For example:

cache = {}
cache['2'] = "two"
cache['1'] = "one"
console.log(cache[2])  #=> "two"
console.log(cache[1])  #=> "one"
console.log(cache)     #=> {1: "one", 2: "two"}

Note the coercion of strings to ints above. This messes with the
ordering of multiselect options list whenever there is a user input. To
avoid this from happening, the keys need to have a string that can't be
coerced automatically, and then preserve the value of the option
element.

I chose to use an object that stores the option value and the option
HTML as the value of the cache and a string of the format 'o_' as the key. This ensures that the insertion order is preserved.
This is the new structure:

cache = {
  'o_271': { id: 271, value: 'CartItem #271'},
  'o_270': { id: 270, value: 'CartItem #270}'
}

The existing structure used an Object as a cache where the keys are the
values of option elements, and values are the HTML content of the option
elements. In Javascript, if a numeric value in the form of a string is
assigned as a key, it gets converted to an integer. Optimization
routines would then order the object to ensure faster access to
elements. For example:

cache = {}
cache['2'] = "two"
cache['1'] = "one"
console.log(cache[2])  #=> "two"
console.log(cache[1])  #=> "one"
console.log(cache)     #=> {1: "one", 2: "two"}

Note the coercion of strings to ints above. This messes with the
ordering of multiselect options list whenever there is a user input. To
avoid this from happening, the keys need to have a string that can't be
coerced automatically, and then preserve the value of the option
element.

I chose to use an object that stores the option value and the option
HTML as the value of the cache and a string of the format 'o_<option
value>' as the key. This ensures that the insertion order is preserved.
This is the new structure:

cache = {
  'o_271': { id: 271, value: 'CartItem railsadminteam#271'},
  'o_270': { id: 270, value: 'CartItem railsadminteam#270}'
}
@meghaarora42
Copy link
Contributor Author

Fixes #2231

mshibuya added a commit that referenced this pull request Sep 28, 2015
@mshibuya mshibuya merged commit 974b572 into railsadminteam:master Sep 28, 2015
@mshibuya
Copy link
Member

Nice try, thanks RailsGirls 😻

@jordanmaguire
Copy link
Contributor

Thanks for this!

@sferik
Copy link
Collaborator

sferik commented Sep 28, 2015

Great work! 🌈 💖 ❤️ 💛 💚 💙 💜 💝

@jordanmaguire
Copy link
Contributor

Here's some thoughts:

  1. What's your thoughts on test coverage? This would be super easy to test with capybara and would prevent regressions
  2. This code would REALLY benefit from some comments explaining why it is necessary to do what has been done. In 6 months or whenever when someone else is looking at this code there is going to be no indication of what this code does. It's good that the info is in the PR, but some of it should be in the code, too.

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

Successfully merging this pull request may close these issues.

4 participants