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

[JENKINS-33495 and JENKINS-33496] Separate scrollspy widget code out from tab widget code #2121

Merged
merged 30 commits into from Mar 18, 2016

Conversation

@tfennelly
Copy link
Member

commented Mar 14, 2016

See JENKINS-33495 and JENKINS-33496.

  • Revert original code committed from #2106. All of the scrollspy js and css/less committed with that PR was in the wrong place i.e. roughed in on top of the tab code. Easier to revert back out and then add back in the right place versus trying to tease them appart by hand.
  • Create scrollspy specific widget components i.e. js, css/less.
  • Add scrollspy tests.

@tfennelly tfennelly force-pushed the tfennelly:JENKINS-33495 branch from a754f0f to 54ff19d Mar 15, 2016

@tfennelly tfennelly force-pushed the tfennelly:JENKINS-33495 branch from fe469df to cfab944 Mar 16, 2016

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2016

@tfennelly tfennelly changed the title [JENKINS-33495] Separate scrollspy widget code out from tab widget code [JENKINS-33495 and JENKINS-33496] Separate scrollspy widget code out from tab widget code Mar 16, 2016

@reviewbybees

This comment has been minimized.

Copy link

commented Mar 16, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Mar 17, 2016

I'm afraid that's more-or-less impossible to make "unweird" since that section is initially visible and then becomes hidden.

Depends on whether the related option in the previous section starts out checked or unchecked.

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2016

Depends on whether the related option in the previous section starts out checked or unchecked.

@daniel-beck ok ... I'm not sure what you mean by that.

ConfigTableMetaData.trackSectionVisibility to now show (as well as hi…
…de) activators for sections that appear
@daniel-beck

This comment has been minimized.

Copy link
Member

commented Mar 18, 2016

Observed in build 4393: The tab bar does not move when the page is scrolled programmatically from the top position until the scroll is finished.

Steps to reproduce:

  1. Go to config page
  2. Click any of the titles (except General)

Expected: Page scrolls down, tab bar "detaches" from its regular location and gets fixed position near the top and stays there.
Actual: Page scrolls down, tab bar disappears until scroll is finished, then it pops up again.

I'm pretty sure this is a recent regression in this PR.

Split watchScroll into 2 scroll listeners, one or auto tab activation…
… and the other for sticking the tabbar to the top of the window.
@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2016

@daniel-beck yes that is different behaviour but has been there since the earliest builds on this PR, trying to fix the jumpiness that you highlighted :)

Anyway ... you forced me to rethink and find a better way of doing it. I just pushed a fix for that.

TBH ... I think we are nit-picking here now at this stage. Can we please get this merged. Can @kzantow and @gusreiber give it bees or not so we can merge it. I'm tempted to just follow KKs lead at this stage and just merge.

@gusreiber you were also confusing me there when talking about other fixes and features that could be added to this stuff (config pages, scrollspy etc). It's standard practise to not talk about other things on a PR and to just keep the conversation fixed on the changes in the PR.

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2016

I'm going to look back here in a few hours and merge this PR if it's not already merged.

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Mar 18, 2016

@tfennelly I don't see a single code review related comment or 🐝/👍 so far. Tyler and I tested it, but I doubt either of us can actually review the changes.

I asked Gus last night to 1. document open issues in Jira so they don't get lost, and 2. find people to actually code review this and approve. AFAICT, neither has happened so far.

@daniel-beck daniel-beck reopened this Mar 18, 2016

body.j-hide-left #main-panel{margin:0 auto; max-width:75em;}
</style>


This comment has been minimized.

Copy link
@recena

recena Mar 18, 2016

Contributor

@tfennelly Thanks for removing that.

}
body.main-panel-only #main-panel {
margin: 0 auto !important;
max-width: 75em !important;

This comment has been minimized.

Copy link
@recena

recena Mar 18, 2016

Contributor

How does it work with? #2029

We should merge #2029 before this.

/cc @daniel-beck

@gusreiber

This comment has been minimized.

Copy link

commented Mar 18, 2016

@daniel-beck @tfennelly Daniel is correct. I need to work through a review in some depth. Will be doing so here today.

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Mar 18, 2016

I've been told @kzantow is currently reviewing this as well. If he amends this PR, I'd appreciate reviews of that by @gusreiber. This way we at least have some review coverage here, and I'm more comfortable merging this.

@kzantow

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2016

I've found a blocking issue: something is causing the draggable behavior to be unusable. This is a regression from what's in the current 2.0 brach. I'm currently investigating this, but I can't give the ok to merge until it's sorted out.

@gusreiber

This comment has been minimized.

Copy link

commented Mar 18, 2016

@kzantow @tfennelly @daniel-beck I see the issue @kzantow is point to. I wrestled with it in my rough version. I will have a fix here shortly.

@kzantow

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2016

Ok, I have a fix for this but I can't seem to actually push to this repo or make a PR against it... kzantow@05a660c

@kzantow

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2016

Ok, I've reviewed this, and agree with Tom we should get it merged in, this PR is too big to add anything further to be able to meaningfully review, and as is I know the pending fix for the only blocking issue and can follow it up immediately.
🐝 👍

@kzantow

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2016

Note: I've created https://issues.jenkins-ci.org/browse/JENKINS-33659 to address the width issues mentioned in this ticket, as they already existed in the version in 2.0 previously and are not regressions.

@tfennelly

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2016

Sorry guys ... I was out in the garden for the afternoon.

@kzantow thanks a bunch for tracking that down and sorry for missing it. I can apply that fix easily and push it.

Applying @kzantow fix for the draggable sections regression
Thanks @kzantow and sorry for missing this!!
@kzantow

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2016

@tfennelly you're welcome :) This was the only blocking issue for me. There are a number of other things mentioned in this PR that are predominantly not related to the changes here but rather the functionality as it existed, which I think can be addressed in follow-on PRs. Either way, I'm 👍 on the overall changes and haven't found anything else... I could nitpick some other things, but it would only serve to muddy up this PR further.

@gusreiber

This comment has been minimized.

Copy link

commented Mar 18, 2016

🐝

@tfennelly tfennelly closed this Mar 18, 2016

@tfennelly tfennelly reopened this Mar 18, 2016

tfennelly added a commit that referenced this pull request Mar 18, 2016
Merge pull request #2121 from tfennelly/JENKINS-33495
[JENKINS-33495 and JENKINS-33496] Separate scrollspy widget code out from tab widget code

@tfennelly tfennelly merged commit 904d9b1 into jenkinsci:2.0 Mar 18, 2016

1 check passed

Jenkins This pull request looks good
Details
@kzantow

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2016

👍 :)

@recena

This comment has been minimized.

Copy link
Contributor

commented on 2cc9853 Mar 21, 2016

I hope to review this when this PR is merged #2029

jsoref added a commit to jsoref/jenkins that referenced this pull request May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.