-
Notifications
You must be signed in to change notification settings - Fork 103
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
Fix Lookup initial change detection #2092
Conversation
canLeave feature needs Behat tests |
Would be nice to have behat tests for canLeave (but then for ALL form controls which is an overkill for this PR to merge) - this should not prevent merging this. If you read FUI documentation, it is clear that lookup was wrongly initialized before which is fixed now. Would be welcomed if you could contribute it if you consider it compulsory here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No change needed for another dropdown "set value" call though in
ui/src/Form/Control/Lookup.php
Line 276 in 4255c21
$chain->dropdown('set value', $row['value'])->dropdown('set text', $row['title']); |
@atk4/trusted-maintainers Can someone help providing the tests that @mvorisek requests for this PR to merge? This is a simple bug introduced which is fixed by the above PR. Very annoying as forms today will always prevent to be left when a lookup control is present. Thank you contributors! |
The change looks correct: docs: https://github.com/fomantic/Fomantic-UI/blob/2.9.3/src/definitions/modules/dropdown.js#L2687 (and https://fomantic-ui.com/modules/dropdown.html#behavior) @mkrecek234 did you manage to get Behat working locally as discussed on Discord? |
Unfortunately no time, @mvorisek - but can confirm that the change is 100% correct, fixing the issue correctly and according to Fomantic UI specs. |
I have merged it, it does not worsen coverage but be aware it can break anytime as no tests. |
fix #2020