Closed batpad closed 1 year ago
Thanks for submitting your first pull request! You are awesome! :hugs:
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! :wave:
Welcome to the Jupyter community! :tada:
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.
@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?
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?
@batpad looking into this a bit, I think the issue is that we define nested
I don't think we need the
@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.
@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.
@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.
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.
@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:
label
- just have a div
enclosing each profile element, and remove this nested label
structure, also because it results in invalid HTML.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.
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.
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?
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.
<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.<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?
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:
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.
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:
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 !
Thanks a lot for engaging on this issue, @consideRatio!
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.
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.
Congrats on your first merged pull request in this project! :tada: Thank you for contributing, we are very proud of you! :heart:
This builds on top of this PR: https://github.com/jupyterhub/kubespawner/pull/724
Fixes #696
Does:
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<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:
Let me know if this works / if there's anything else that would be good to do here.
cc @yuvipanda