Select profile if any of its choices are interacted with

[batpad]

This builds on top of this PR: #724

Fixes #696

Does:

• Adds js- prefixed classes to the Select boxes and the Label containers - this is not strictly necessary, but I find it good practice to separate out classes used by Javascript
• Adds a <script> tag to the end of the template with simple jQuery to react to changes to the select tag and ensure that the parent Input Radio is selected. If there's a strong preference, can move to a separate file right away, or can do that if and when the JS gets more complex.

Stylistic choices:

• I've gone ahead and used jQuery since it existed - happy to remove the dependency, but it would mean a bit more verbose code.
• There's probably a few different ways to handle traversing the DOM and selecting the correct radio, probably with different trade-offs. I've gone for the simplest approach here: this does mean that this function would need to be looked at in case the HTML structure of the page drastically changes, but hopefully it's simple enough to tell what's going on here that it would be easy to fix.

Let me know if this works / if there's anything else that would be good to do here.

cc @yuvipanda

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[yuvipanda]

Hmm, I was trying to see why pre-commit.ci couldn't auto-fix https://results.pre-commit.ci/run/github/46604988/1683023436.r2OoP9CCQH6uz9H7a_tv_g.

Wonder what is the cause here.

[consideRatio]

@yuvipanda pre-commit.ci and we as maintainers doesn't have rights to push commits to this branch and that is whats caused the failure in pre-commit.ci.

@batpad can you do pip install pre-commit and pre-commit run -a and push a commit with the autoformatting changes?

[consideRatio]

Wieee thank you @batpad for working this!!
I really appreciate the js- convention for things interacted with via js - that certainly helps overview what goes on better, thanks for introducing it!

I saw that this solution doesn't handle all interactions, just when something is changed. Do you have ideas on how we can select the radio button through any kind of interaction - even if its not changing something?

[consideRatio]

@batpad looking into this a bit, I think the issue is that we define nested tags, which doesn't seem to be something thats considered okay to do, see https://stackoverflow.com/a/20842000/2220152.

I don't think we need the tag on Image / Memory / CPU above. Maybe its sufficient with <span> or <p> or similar?

[batpad]

@consideRatio thanks for looking at this!

Ah, I see, so if you click on the option but dont actually "change" it in the Select, it doesn't select the Profile, when it probably should.

This is because we're currently binding to the "change" event on the select, and since it does not change, the event does not get fired.

Do you think it's okay to trigger the selection of the profile just when the user clicks the Select box? i.e. don't wait for Select to open and click on an Option. (I'll submit a quick commit so we can see what this looks like)

We can try this with binding to the click event on the Select - if that seems awkward, let me see if there's any way to have an event on clicking an option, even if the option does not change the selection.

[consideRatio]

@batpad ah yes! I think click is an improvement solving the "not changed, but interacted" issue.

I also think pressing the dropdown labels should work, and that can be fixed by updating the for attribute of them to point to the radio button id profile-item-{{ profile.slug }} instead of the specific dropdown option id.

[batpad]

@consideRatio I pushed a commit to change the profile selection to happen on the click event. So now, the profile should be selected when you click on the Select box, regardless if you change the option or not. Thanks for that catch.

Interestingly, the behaviour for me is slightly different on Chrome and Firefox.

On Firefox: Profile Radio gets selected AFTER clicking on the option.

On Chrome: Profile Radio gets selected immediately when clicking on the Select.

Let me know if it helps to attach a video of the behaviour - although I think this fulfills the purpose of just making sure the Profile radio gets selected whenever someone clicks on the profile or interacts with it.

[batpad]

I also think pressing the dropdown labels should work

Ah, another really good catch :-) - lemme get to that. Yes, ideally this can be fixed with the for attribute, otherwise will add another click handler for the label or so.

[batpad]

@consideRatio hmm I pushed a commit to add the click handler to the label - this "works" and I guess is okay, but just trying to take a step back on the problem a bit --

Also reading the StackOverflow issue you linked. Right, so the fundamental problem here was that we were trying to make the click work just using the label and for attributes. This will not work for nested form elements - it seems like a nice way to avoid some Javascript :-) - but I think what we might actually want to do here:

• Change the structure to not wrap the entire element in a label - just have a div enclosing each profile element, and remove this nested label structure, also because it results in invalid HTML.
• Just attach a single click handler to the parent div element to select the input radio inside.

This seems to me like it would be a bit cleaner - result in cleaner markup, and if we need to have some JS anyways to make this work, it seems cleaner to have a single click event on the container.

Let me know if this makes sense and this should actually be pretty quick to do.

[consideRatio]

This is fine as it is, if you want you can go for the for change and reduce what's done by the JS but I'm not sure I think that is better as the label really is for the select box - its just that we change it to hack the selection logic when using nested labels which we shouldn't.

So, I suggest we stay with what we have here.

But! Autoformatting - run do pip install pre-commit and pre-commit run -a, then commit those changes. You can also do pre-commit install --install-hooks to get autoformatting changes made before you make commits going onwards.

[batpad]

I don't think we need the tag on Image / Memory / CPU above. Maybe its sufficient with <span> or <p> or similar?

So I tried this with a span and that works so that the input gets selected when clicking the label.

Since label inside label is invalid according to the spec, I can just make that change, and maybe this is okay to go? I think it might be better to stay away from larger restructuring of the HTML?

[batpad]

This is fine as it is

Oops, this is the second time I've seen your comments a bit late :-) - but I think with the latest commit doing the autoformatting I think this is good to go.

• Changed the <label> to a <span> to remove the nested labels and changed around the JS accordingly, so we don't need that extra click handler and don't have nested labels.
• Had to give the <span> a class to apply the CSS, and unfortunately .label didn't work because that conflicted with some .label class somewhere else, so just called it profile-options-label.

If we do want to keep the label for the select to be semantically correct, happy to do that as a separate PR and restructure the HTML a bit, but I think this is probably fine for now?

[consideRatio]

With the change to <span> the styling fails like this:

Do you think we can accomplish this without styling changes?

For comparison, this is how it looks in main branch:

[consideRatio]

Btw I don't think we need to fix the nested label tag issue, it seems to work after all - so if it comes to retaining the styling vs stop nesting labels - then I favor retaining the styling as it was.

[batpad]

Btw I don't think we need to fix the nested label tag issue, it seems to work after all - so if it comes to retaining the styling vs stop nesting labels - then I favor retaining the styling as it was.

Haa, apologies for not catching the styling regression in Chrome - so strange that the styling is fairly different in Firefox and now it's driving me a bit mad trying to figure out what's happening exactly :-)

And apologies that what should have been a simple fix has spiraled a bit, but thanks again for your feedback and watchful eye. I'm going to play around a bit with the styling mostly to satisfy my own curiosity on what's happening here, but I think you're right: it's better to revert to the nested labels, since that all seemed to work. I do prefer adding the explicit click handler rather than adding the for for the parent radio for two reasons:

• Am pretty sure am seeing inconsistent behaviour when clicking on it between browsers, and since this is undefined in the spec, I'd rather not depend on non-standard browser behaviour for that to work.
• It does seem like a misuse of the label tag and seems like it would really mess with things like screen readers, with the label claiming to be a label for something it's not really the label for.

I have to say that there's a part of me that still really does not like the nested labels, and inconsistencies I'm seeing between chrome and firefox are worrying me a bit. But I think we can take that up separately and focus this PR on just fixing the issue it set out to fix, which is simply selecting the profile when the "Select" is interacted with.

Will revert to nested labels with the click handler on the label and push in a bit.

Thanks again @consideRatio !

[yuvipanda]

Thanks a lot for engaging on this issue, @consideRatio!

[batpad]

Reverted to using the label tag instead of span and adding the click handler.

@consideRatio turns out the styling was totally an Oops on my part: I accidentally removed the form-control class on the <select>. So we could do either label or span here - switching to span will allow us to remove the additional click handler.

Have left it at label for now based on our last discussion. If this looks okay to you, I'd be fine with merging this and happy to chat about the nested label stuff separately if it's something we think is worth changing down the line.

[batpad]

so strange that the styling is fairly different in Firefox and now it's driving me a bit mad trying to figure out what's happening exactly

Just to clarify - this was just my brain on too little coffee or so - I accidentally removed a class and then my brain got confused - have tested between FF and Chrome again and things look consistent / fine. Sorry about that, and thanks again for catching.

[consideRatio]

Wieee this looks exactly like before testing on chrome, and works exactly as expected to fix #696.

I opened #734 about avoiding nested <label> tags as it seems you have a quick followup fix for that without introducing a styling change! Nice!

Thank you for your thorough work on this @batpad!! ❤️ 🎉 🌻

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️