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

theme-f1 chromeless logs template updates #6965

Conversation

benjaminapetersen
Copy link
Contributor

  • Update markup in chromeless templates to fix log scrolling issues.

At some point in the near future we should encapsulate the scrolling functionality better. It's fairly brittle & prone to breaking w/o being noticed. I left comments in the chromeless-build-log.html file for future reference.

Checklist of bugs intending to addressed:

  • bug 6956 scroll top & bottom broken on chromeless logs
  • bug 6985 log keeps following on manual scroll
  • bug 6957 hamburger menu on chromeless
  • bug 7003 affix while scrolling
  • fix broken alignment of follow link (right aligned)
  • fix alerts on the chromeless log page
  • remove log-chromeless class

So to let github handle closing issues when merged:
fixes #6956
fixes #6985
fixes #6957
fixes #7003

@sg00dwin @spadgett

@benjaminapetersen
Copy link
Contributor Author

Fixes issue #6956, is a follow up to this PR

@spadgett
Copy link
Member

spadgett commented Feb 2, 2016

This might have been introduced by #6663, but the log warning needs some padding.

openshift_web_console_and__log-viewer_html____go_src_github_com_openshift_origin_assets_app_views_directives_logs__-vim_and_theme-f1_chromeless_logs_template_updates_by_benjaminapetersen pull_request__6965 _openshift_origin_and_failed_t

@@ -1,17 +1,27 @@
<default-header class="top-header"></default-header>
<div column flex class="content">
<div flex class="log-chromeless">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove log-chromeless from the less file if not used.

@spadgett
Copy link
Member

spadgett commented Feb 2, 2016

Should this link be flush to the right?

openshift_web_console_and__log_less____go_src_github_com_openshift_origin_assets_app_styles__-vim_and_theme-f1_chromeless_logs_template_updates_by_benjaminapetersen pull_request__6965 _openshift_origin_and_failed_to_open_page

@spadgett
Copy link
Member

spadgett commented Feb 2, 2016

The follow link positioning doesn't seem to be specific to chromeless logs. Is that where it was before?

@jwforres
Copy link
Member

jwforres commented Feb 2, 2016

@spadgett no it was definitely right-aligned before

@spadgett
Copy link
Member

spadgett commented Feb 2, 2016

I think it was because we removed the log-chromeless class.

.log-chromeless {
  margin-top: -1px;
  .log-size-warning {
    margin: 10px;
  }
  .log-view {
    .log-scroll-top.affix {
      right: 0;
    }
  }
}

@benjaminapetersen
Copy link
Contributor Author

Yeah thats weird. I'll look at that as well.

@benjaminapetersen
Copy link
Contributor Author

The right alignment is an easy fix. However, the "sticky" behavior is tricky because it uses bootstrap affix internally, (<div affix offset-top="445" offset-bottom="90") but may need passthrough configuration on the log-viewer directive. 😕

@spadgett
Copy link
Member

spadgett commented Feb 2, 2016

@benjaminapetersen we can fix the sticky behavior in another PR

@benjaminapetersen benjaminapetersen force-pushed the bpeterse/theme-f1-chromeless-logs-fix branch from 724ca0e to a302df7 Compare February 2, 2016 21:19
@benjaminapetersen
Copy link
Contributor Author

@spadgett sounds good, will pass on that. I had already done a minor update to the affix directive, but will leave it as is at this point.

@@ -76,6 +77,10 @@
}
}

.chromeless .log-scroll-top.affix {
right: 0px; // override
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is .log-chromeless still defined? We should remove that and carry over the style that adds margin to the log length warning.

@spadgett
Copy link
Member

spadgett commented Feb 3, 2016

Let's open an issue to track the affix problem.

@benjaminapetersen
Copy link
Contributor Author

(moving list to description)

@spadgett
Copy link
Member

spadgett commented Feb 4, 2016

If you put Fixes #<number> in the description, it'll automatically close the issue when merged. I think it needs to be in a description, not a comment.

@benjaminapetersen
Copy link
Contributor Author

ah, nice. ok ill update that.

@benjaminapetersen benjaminapetersen force-pushed the bpeterse/theme-f1-chromeless-logs-fix branch from a302df7 to 0771431 Compare February 4, 2016 19:07
@benjaminapetersen
Copy link
Contributor Author

@spadgett @jwforres @sg00dwin ready for review.

Only thing to note is affix has at least one lingering issue. It now functions again on normal/chromeless views, and works mobile & non-mobile. However, on a resize between mobile-non-mobile, it struggles to continue to work properly. A page refresh will get it working again. I tried a number of different things but wasn't able to find a workable solution to this (note resize to mobile "breaks", but resize back to full view will work again, or vice-versa).

@benjaminapetersen
Copy link
Contributor Author

Oh let me kill that css line & fix that alert padding... got excited about affix & almost forgot those.

}
});
}
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried adding $affixableNode.affix('checkPosition')?

This method needs to be called whenever the dimensions of the affixed content or the target element are changed, to ensure correct positioning of the affixed content.

http://getbootstrap.com/javascript/#affixcheckposition

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. It doesn't register the target update so that causes more weird positioning.

Also tried .data('bs.affix').options.target per this SO article. Was optimistic with this one since the fix suggested is by one of the BS creators 😄

@jwforres
Copy link
Member

jwforres commented Feb 4, 2016

@smarterclayton for 1.1.2

attachScrollEvents();
updateScrollLinks();
affix();
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to pass a value for wait to debounce?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it was 50 before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oop, yeah, good catch. Don't want to miss that. it defaults to 0 if just wanting to kick it up the stack, but def want a real delay here. fixing. Was 50, setting to 100. 200 even seemed reasonable when testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@benjaminapetersen benjaminapetersen force-pushed the bpeterse/theme-f1-chromeless-logs-fix branch from 0771431 to d529a0d Compare February 4, 2016 21:12
@benjaminapetersen
Copy link
Contributor Author

@spadgett fixed the alerts w/ @sg00dwin, btw. Forgot to comment that earlier.

screen shot 2016-02-04 at 4 09 34 pm

- fixes hamburger menu collapsing on chromeless pages
- fixes follow/end link placement on chromeless pages
- provides pass-through configuration option on log-viewer for underlying affix directive
	- TODO: more work is needed to get this in all contexts, but mobile seems to function
- fixes a handful of the affix issues
- optimizes DOM manipulation while logs running
- fix alert padding on chromeless page
@benjaminapetersen benjaminapetersen force-pushed the bpeterse/theme-f1-chromeless-logs-fix branch from d529a0d to d9f2b73 Compare February 4, 2016 21:21
@spadgett
Copy link
Member

spadgett commented Feb 4, 2016

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/829/) (Image: devenv-rhel7_3339)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to d9f2b73

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to d9f2b73

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/829/)

openshift-bot pushed a commit that referenced this pull request Feb 5, 2016
…omeless-logs-fix

Merged by openshift-bot
@openshift-bot openshift-bot merged commit 82cff46 into openshift:master Feb 5, 2016
@benjaminapetersen benjaminapetersen deleted the bpeterse/theme-f1-chromeless-logs-fix branch February 5, 2016 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants