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

Fixed issue #518: Textarea and select value isn't preserved #542

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

the-ress
Copy link

Fixed issue #518: Textarea and select value isn't preserved when going back/forward

@mislav
Copy link
Collaborator

mislav commented Jul 17, 2015

Hi thanks for the contribution. This would need automated tests in order to be accepted. The tests should also verify whether regular <input> fields have their values preserved (I assume they have by default?).

Does this handle <select> boxes with multiple values? Also, could this adversely affect our caching functionality somehow?

@masone
Copy link

masone commented Sep 23, 2015

Given this PR is already a few months old, I have decided to jump in and help with the tests, see https://github.com/masone/jquery-pjax/commit/f9c06149f6058a1fd440d3e885c4d225a9e58e29. I can confirm that <textarea> and <select> do not preserve their values, but other form elements do. There are now 5 failing tests on my branch.

When testing @the-ress' changes, the one for the single <select> is still failing. I'm not sure wether your code or my tests need additional work, it would be nice to have a second pair of eyes on it. Please note that the assertions for the multi are slightly different from the single which might lead to one test failing while the other works.

I'm new to jquery-pjax, so please have a thorough look before merging my changes.

@the-ress
Copy link
Author

Thank you for the tests. I never managed to set up the environment to be able to run them.

The single value select was a bug in the original PR. I fixed it and merged your test branch.

@mislav
Copy link
Collaborator

mislav commented Sep 23, 2015

@the-ress I'm sorry you had problems running the test suite locally. You need to have Ruby with Bundler installed and run script/bootstrap. Then, you can run script/server -p 3000 (or another port of your choise) and open http://localhost:3000 in your browser.

@@ -562,6 +562,21 @@ function uniqueId() {
}

function cloneContents(container) {
// Preserve textarea values
var textarea = findAll(container, "textarea")
textarea.text(function(i, text){return textarea[i].value})
Copy link
Contributor

Choose a reason for hiding this comment

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

Isnt this missing a .each?

Copy link
Author

Choose a reason for hiding this comment

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

No, text() has similar functionality: http://api.jquery.com/text/#text-function

@masone
Copy link

masone commented Sep 24, 2015

Ugh sorry, I messed up the git history on the temporary form_tests_theress branch. I thought you would merge the form_tests instead. Please squash the commits before you finish this PR 🙈 😄

elem = $(elem);
var values = $(elem).val();
values = $.isArray(values) ? values : [values];
elem.find('option[selected]').attr('selected', false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not happy that this performs DOM changes on elements before they are cloned. How about just making these changes on the elements after they have been cloned?

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