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
Javascript libraries update #677
Conversation
A few comments from looking at https://hydra.sharcnet.ca:
|
73a744b
to
fbdf6fe
Compare
Thanks. Fixed now. I also created you a temporary account on the server so you can fully dig through all the pages, dialogues, and such. Let me know if you find anything else. |
fbdf6fe
to
b439c28
Compare
@edolstra expect you have been just busy with things, but it's been 4 months now, so just wondering what the thoughts are on this? We've been running it on our server and haven't noticed any issues yet. Noticed a minor conflict had arisen against master, so just finished resolving that. Thanks! |
Actually, I just finished breaking our hydra server by pushing this branch to it to double check my conflict resolution. Looks like it is just a newer perl dependency ( I probably won't get it resolved to tomorrow though. |
Updated the package dependencies for the newer version and now our hydra server is back up for your perusal. Thanks. |
@edolstra any chance this could get merged if I fixed the latest set of conflicts that have arisen in the since I lasted fixed conflicts back in January? It looks like there was a branch created back in 2013 to attempt to update to boostrap 3, so I was thinking it might have been something you were wanting. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/calling-all-hydra-users/11851/5 |
I gave rebasing this a shot, and published my rebase here: https://github.com/grahamc/hydra/pull/new/twhitehead-javascript-updates. I took a look at this with @samueldr and we found a number of issues. Most of them are minor, but one is quite major. Most significantly: clicking tabs don't update the URL.For example, visit https://hydra.nixos.org/build/137117854 and select Details/Inputs/etc. It'll update the URL, and you can deep-link to a page. Bootstrap 4 seems to eliminate that behavior. Ideally there is a way to bring it back without too much work, but I found https://webdesign.tutsplus.com/tutorials/how-to-add-deep-linking-to-the-bootstrap-4-tabs-component--cms-31180 and it is pretty nasty. I think this problem is significant enough to block this PR. Important if the deep-linking is fixableEntering a
Less important things that can be easily fixedThese can probably be each fixed by search and replace. Button stylingSome button widgets are missing the buttoney display treatment. Example from a build page: Error / warning label stylingSome labels like warnings and errors are no longer looking like labels. Example from a project page: Project page's default tab state isn't correctOn a project page like Bugs that are already bugs that I don't expect this PR to fix...but are maybe a bit more pronounced with the new styles. Zebra stripes on the project list and jobset list pages are mis-striped due to hidden jobsets #578vs: More button & Jobs header on a jobset don't extend far enoughOverall, I think if we can fix the deep linking problem we could merge this. The new style is definitely roomier than the old style, but @samueldr pointed out that it is also much more touch-friendly. It took a few minutes to adjust to the new size, but I don't mind it and actually kind of like it by now. |
The w3 site has a bit of javascript to restore deep linking functionality.
I'm not terribly familiar with javascript, but my reading would be
|
This is not "the w3 site"... Sorry for the nit pick. It's just an arbitrary website full of stolen snippets and tutorials. It comes, word for word AFAICT from this bootstrap issue: This is not a good thing. This is re-implementing what was purposefully broken by bootstrap 4. The links already points to anchors. This may work in most browsers, but I wouldn't put any trust in patching on top of the breakage bootstrap 4 did. I wouldn't feel confident in approving something that broke a fundamental behaviour of the web, even with a patch on top to try and reinstate it. Furthermore |
@samueldr Thanks for the details and sorry for the bad link. I have to confess I am really out of my depth when it comes to this sort of thing. From your comments and the twbs/bootstrap#25220 issue, it looks like this would be more correct/cleaner
With regard to the fundamental behaviour of the the web, I'm not sure I understand what you are saying. I am guessing maybe you are saying the basic idea of using anchors to deep link tabs is breaking a fundamental behaviour of the web, and that is why it was deliberately removed in bootstrap 4. Is this it, and, if so, are you then saying we shouldn't have deep linking period? |
Don't worry, the web is way way way more complex than it looks like. So being out of one's depth is not necessarily an issue :).
What I meant to say is that clicking on an anchor link should push the navigation to that anchor in the browser history, and scroll to the anchor. I guess scrolling to the anchor is undesirable with tabs navigation. Though looking at how it works, now, I see that @edolstra basically implemented it. It looks like bootstrap 2 would, too, have broken the navigation the same way. So in the end I guess it's not an issue if we fix it in post? uh. (For the record, I'm not a fan of bootstrap, but I'm still surprised when bootstrap drops the ball...) And yes, on their example page for bootstrap 2 you can see it's broken. So uh... Looks like we want this to be fixed, which is the initial assumption that holds, but it's fine to do it "in post", which is my assumption that was flawed. |
This option looks great, and resolves my largest concern: $(`a[href="${window.location.hash}"]`).tab('show')
$('a[role="tab"]').on("click", function() {
history.pushState(null, null, url.split("#")[0] + $(this).attr("href"));
}); |
Thanks guys. Interesting that the original behaviour appears to have been JavaScript too. In some sense then it was a bit I missed in my update. I'll try the rebase plus the javaScript on our server next week to see how it goes. |
Co-authored-by: Graham Christensen <graham@grahamc.com> ... but just fixing up merge conflicts from the introduction of flakes and the removal of the Jobs table.
Okay, I think I've got the deep link stuff working. It is live on our hydra server, so give it a try and let me know. I'll force push an update. I went through the rebase and made some other tweaks on top of it too. So I'll add some comments about that. If it all looks good, I'll squash the fixups and push it again. |
b439c28
to
3abe538
Compare
<div class="btn-group btn-group-toggle col-sm-9" data-toggle="buttons"> | ||
<label id="type-flake" class="btn btn-secondary[% IF jobset.type == 1 %] active[% END %]"> | ||
<input type="radio" name="type" value="1" [% IF jobset.type == 1 %]checked[% END %]>Flake</button> | ||
<input type="radio" id="editjobsettype" name="type" value="1" [% IF jobset.type == 1 %]checked[% END %]>Flake</button> |
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.
Was missing id
to tie back to label.
@@ -6,7 +6,7 @@ | |||
<div class="form-group row"> | |||
<label class="col-sm-3" for="editprojectenabled">Enabled</label> | |||
<div class="col-sm-9"> | |||
<input type="checkbox" id="editprojectenabled" name="enabled" [% IF project.enabled %] checked="checked" [% END %]/> | |||
<input type="checkbox" id="editprojectenabled" name="enabled" [% IF create || project.enabled %] checked="checked" [% END %]/> |
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.
The original had create
, so added it back. Let me know if this was removed on purpose.
With respect to the The That is just me though. I'm perfectly happy with whatever you guys want. |
Should I strip out as much of that commit as possible, or just the |
Only the |
I'm excited for this PR, I think once this last bit is sorted we can go ahead and merge. |
absolutely no one said:
Well, it could be. It's not a good idea. See bugs websites have to work around. Keeping it at the top rather than stickied makes a class of bugs and issues entirely vanish. Thanks for coming to my ted talk. |
I didn't even notice the difference. =) |
Okay. I removed the I think the only big looking difference is that the error stuff is now white on darker red instead of darker red on light red. You can checkout our hydra server as I've updated it to see what you think. How would you guys want all these commits to merge them? Are you going to do a merge squash so it doesn't matter, or should I
|
By everything, I mean all the tweaks we have made to the bootstrap update commit. I would still leave the other javascript library update commits separate. I would of course list you guys as co-authors. |
I wouldn't put the evaluation errors on red. This is because "Evaluation errors" is rather vague, and in Nixpkgs' case, things are fine even with the errors. So styling them the same exact way as other logs is probably better. @grahamc thoughts about the evaluation log? |
I agree, the white on red is too intense. It seems like that style is optimized for very very short errors, but the evaluation error log should be optimized for reading. w.r.t. commits: I'm inclined to do a squash and merge. |
And more importantly, crucially, fix the "appended" button so the round corners are on the right (heh) side.
5774ec0
to
e51a6a4
Compare
I've gotten rid of the white on red, and expanded out the history on the two fixups commits I did, so that is everything I am aware of/wanted to do. I've updated our hydra sever with the latest as before. Unless anyone else has anything else outstanding on this, I think it is clear to squash merge. Thank both of you very much for all the feedback, commits, and just general work pushing this one though. 👍 |
Thank you so much for coming back to this PR and getting it across the line, @twhitehead! |
Not a problem at all. You guys made it super easy between the rebase and followup patches! 😁 |
Update to newer versions of the various javascript libraries used.
I undertook this as our security people flagged our SHARCNET/Compute Canada hydra server as a XSS security risk due to the use of jquery 1.x. I'm not certain I agree XSS concerns apply to hydra, but figure the update was worthwhile in its own right as the libraries had fallen multiple major versions behind.
You can check out our hydra server to see a running version of these patches on top of the hydra server packaged in 19.03. I rebased that patch set onto master for this pull request. I haven't actually tried it, but the rebase was 100% clean except for a missing
c.uri_for
call that had been added.