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.

Sign up for GitHub

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

Jump to bottom

Trying to fix #1916 #1918

Merged
merged 6 commits into from
Oct 31, 2017
Merged

Trying to fix #1916 #1918

merged 6 commits into from
Oct 31, 2017

Conversation

robinchew
Copy link
Contributor

Select change event does not update options correctly. This fix will correct it but test fails.

Issue #1916

@robinchew robinchew requested a review from lhorie as a code owner July 28, 2017 08:12
@dead-claudia dead-claudia requested review from pygy and dead-claudia and removed request for lhorie July 28, 2017 08:13
@botvac
Copy link

botvac commented Jul 28, 2017

Warnings
⚠️

Please add an entry to docs/change-log.md.

Generated by 🚫 dangerJS

@dead-claudia
Copy link
Member

dead-claudia commented Jul 28, 2017 •
edited
Loading

Your error is running into this limitation, but it's not your fault. @pygy Me thinks this is a good time to fix it? It seems a bit simpler than what we currently have, ironically enough.

@pygy
Copy link
Member

pygy commented Jul 28, 2017

@isiahmeadows I'll look into the DOM spec details this weekend. I don't think the mocks have a concept of dirtiness for options, and currently setting the value of a select doesn't change the selected attributes of the children.

@dead-claudia
Copy link
Member

If I'm reading the spec correctly, I think the concept of dirtiness is really just a (IMHO poorly named) locking mechanism for the selected HTML attribute (as opposed to the IDL-side option.selected getter/setter). If we opt to not support the attribute, I'm not sure we need to implement the "dirtiness" value.

@dead-claudia
Copy link
Member

@pygy Can we merge this PR first, adding a tracking issue for the broken options.selected, and then once one of us gets this magical thing called time (:smile:), come back to fix it?

@tivac
Copy link
Contributor

tivac commented Aug 31, 2017

If this gets merged w/ failing tests it's gonna need someone committing to fix the tests soon after. I think the PR is fine, but I'm really uncomfortable with leaving the build broken.

@dead-claudia dead-claudia mentioned this pull request Oct 1, 2017
Closed
2 tasks
@robinchew robinchew requested a review from tivac as a code owner October 25, 2017 16:46
@dead-claudia
Copy link
Member

@pygy @tivac Are you okay with this?

BTW, I instructed @robinchew on Gitter to include the commented out test because I plan to migrate most of the tests to run on the DOM as well if it's available (better test coverage), and I'm more likely to remember to test for this particular bug if it's already there.

@pygy
Copy link
Member

pygy commented Oct 29, 2017

@isiahmeadows let's merge it with a stubbed test, yes (a failing test in next would be a pain to work around while working on other aspects of the code base).

@dead-claudia
Copy link
Member

@pygy So beyond that, does this PR look okay to merge?

@pygy
Copy link
Member

pygy commented Oct 31, 2017

@isiahmeadows I'd say yes

@pygy pygy merged commit db2a12d into MithrilJS:next Oct 31, 2017
pygy added a commit to pygy/mithril.js that referenced this pull request Dec 8, 2017
@pygy
render: fix perf regression introduced by MithrilJS#1918
9eda8f1
@pygy pygy mentioned this pull request Dec 8, 2017
Merged
10 tasks
pygy added a commit to pygy/mithril.js that referenced this pull request Dec 8, 2017
@pygy
render: fix perf regression introduced by MithrilJS#1918
1782fa8
dead-claudia pushed a commit that referenced this pull request Dec 9, 2017
Merge pull request #2052 from pygy/fix-perf-regression
1ad9f84 
render: fix perf regression introduced by #1918
@dead-claudia dead-claudia mentioned this pull request Oct 9, 2018
Closed
7 tasks
dead-claudia added a commit to dead-claudia/mithril.js that referenced this pull request Oct 12, 2018
@dead-claudia
Trying to fix MithrilJS#1916 (MithrilJS#1918)
f4d0c3b 
* Trying to fix MithrilJS#1916

* Remove test for rendering select options. Add back after resolving issue MithrilJS#1978.

* Add MithrilJS#1916 fix to change log.

* Revert "Remove test for rendering select options. Add back after resolving issue MithrilJS#1978."

This reverts commit d4c1be7.

* Comment on why failing test for MithrilJS#1916 is commented out.
dead-claudia added a commit to dead-claudia/mithril.js that referenced this pull request Oct 12, 2018
@dead-claudia
Merge pull request MithrilJS#2052 from pygy/fix-perf-regression
d1a8911 
render: fix perf regression introduced by MithrilJS#1918
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pygy pygy pygy approved these changes

@dead-claudia dead-claudia dead-claudia approved these changes

@tivac tivac Awaiting requested review from tivac

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@robinchew @botvac @dead-claudia @pygy @tivac