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

Protostar BC Break - Multi-Selects #20606

Closed
tonypartridge opened this issue May 29, 2018 · 9 comments
Closed

Protostar BC Break - Multi-Selects #20606

tonypartridge opened this issue May 29, 2018 · 9 comments

Comments

@tonypartridge
Copy link
Contributor

tonypartridge commented May 29, 2018

We had this code output for years:

<div class="checkbox btn-group ">
		    <label for="cb_wd0" class="checkbox btn active btn-danger"><input type="checkbox" id="cb_wd0" name="weekdays[]" value="0" onclick="updateRepeatWarning();" class="checkbox " style="display: none;"><span class="sunday">S</span></label>
<label for="cb_wd1" class="checkbox btn active btn-success"><input type="checkbox" id="cb_wd1" name="weekdays[]" value="1" onclick="updateRepeatWarning();" class="checkbox " style="display: none;">M</label>
<label for="cb_wd2" class="checkbox btn active btn-success"><input type="checkbox" id="cb_wd2" name="weekdays[]" value="2" checked="checked" onclick="updateRepeatWarning();" class="checkbox " style="display: none;">T</label>
<label for="cb_wd3" class="checkbox btn active btn-success"><input type="checkbox" id="cb_wd3" name="weekdays[]" value="3" onclick="updateRepeatWarning();" class="checkbox " style="display: none;">W</label>
<label for="cb_wd4" class="checkbox btn active btn-success"><input type="checkbox" id="cb_wd4" name="weekdays[]" value="4" onclick="updateRepeatWarning();" class="checkbox " style="display: none;">T</label>
<label for="cb_wd5" class="checkbox btn active btn-success"><input type="checkbox" id="cb_wd5" name="weekdays[]" value="5" onclick="updateRepeatWarning();" class="checkbox " style="display: none;">F</label>
<label for="cb_wd6" class="checkbox btn active btn-success"><input type="checkbox" id="cb_wd6" name="weekdays[]" value="6" onclick="updateRepeatWarning();" class="checkbox " style="display: none;"><span class="saturday">S</span></label>
		</div>

In short it is a multi-day selector allowing the user to tick click the boxes. However, since PR: #20224 These now become single selects only allowing them to be selected once.

@Quy
Copy link
Contributor

Quy commented May 29, 2018

Pinging @okonomiyaki3000

@okonomiyaki3000
Copy link
Contributor

@tonypartridge I need to know what updateRepeatWarning() is doing. For example does it stop propagation or prevent default?

@okonomiyaki3000
Copy link
Contributor

I think something else may be to blame for what's happening here. If I replace template.js with a much older version (like https://raw.githubusercontent.com/joomla/joomla-cms/3.7.5/templates/protostar/js/template.js) I still get the same behavior.

Also, as long as they have the 'active' class, the code in template.js doesn't affect them (I removed it for testing).

@okonomiyaki3000
Copy link
Contributor

Also, just FYI, if a label encloses its input, it doesn't need a for attribute and the input doesn't need an id. In case you want to simplify that html a little.

@okonomiyaki3000
Copy link
Contributor

@tonypartridge I just tried checking out older and older versions to see if I could determine where the change actually happened. I got as far as April 2017 and the behavior is the same as now (only one label shown as active at a time). I agree this doesn't make any sense for checkboxes but it seems it was always wrong. I suspect that, if it was working in a correct way for you before, it was some of your own code that was making that happen. Then, if #20224 did interfere with your code, it may be due to the fact that I switched to handling the click event through delegation instead of directly on the object where it occurs. This should not cause a problem in 99% of cases but it is a slightly different way of handling things so it's possible the problem is related. I would not like to go back to the old way though. Delegation is just a much better way of doing this.

If I know more about any additional js running on your page, I can determine whether or not my guess is correct and maybe I can help you fix it.

@tonypartridge
Copy link
Contributor Author

@okonomiyaki3000 if you revert your PR and instead use the original which is:

https://github.com/joomla/joomla-cms/blob/be070184e705e367577bcbcc65e099a24cdf7a6a/templates/beez3/javascript/template.js

Clear your cache obviously everything works again using the provided HTML.

updateRepeatWarning() is just a notice handler with a simple alert message for our component.

@okonomiyaki3000
Copy link
Contributor

@tonypartridge Beez3? I thought this was about protostar?

@okonomiyaki3000
Copy link
Contributor

@tonypartridge could you clarify? Everything you've said up until now has been about Protostar but the link you've given is for Beez3. Also, as I said, I reverted the file (and all files) back as far as their state from April 2017 and got the same behavior. You probably need to give a full listing of all javascript (and maybe css) that is loaded on the page where this is working.

@tonypartridge
Copy link
Contributor Author

@okonomiyaki3000 sorry you are right I've got the wrong PR.. the issue is protostar I'll look into it properly tomorrow and do a new issue if I get time.

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

No branches or pull requests

5 participants