Skip to content

Commit

Permalink
Review fixes:
Browse files Browse the repository at this point in the history
- Restore default padding, add specialized class and selector to let the source
  listing use reduced padding for "full bleed" display to the edges of the body.
- Specialize highlighted line display to also include the faux bottom border.
- Specialize blame gutter to also include the faux bottom border.
- Refactor the faux bottom border stuff to use CSS variables so that it's less
  work to change the colors.
- Make the sticky logic not get confused by diff lines without a line number.
- Restore the blame pop-up to not live under the role=button.
  • Loading branch information
asutherland committed Jun 11, 2019
1 parent 870b420 commit 466d7e0
Show file tree
Hide file tree
Showing 5 changed files with 88 additions and 16 deletions.
68 changes: 62 additions & 6 deletions static/css/mozsearch.css
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,33 @@
font-style: normal;
}

:root {
/* ### Highlighting ### */
/* Selected source line background color. This is used when clicking on a
line-number (optionally holding ctrl or shift) to select it/a range. */
--highlighted-bgcolor: rgb(255, 255, 204);

/* ### Sticky Headers ### */
/* The background color applied to code lines that start a block and stick
around until the block is closed. (They have the "stuck" class applied.) */
--sticky-header-bgcolor: #ddf;
/* The color of the faux border line at the bottom of the last stuck line.
In order to avoid reflows, the line isn't actually a "border", but rather
a line faked via linear gradient. */
--sticky-header-bottom-border-color: blue;
/* The gradient transition points. For a solid line with a hard break,set
both values the same, for some kind of smoother gradient, spread them out.
(And maybe rewrite the gradients :) */
--sticky-header-bottom-border-off1: 90%;
--sticky-header-bottom-border-off2: 90%;

/* ### Blame ### */
/* Static blame zebra stripe colors. This may change in the future to be
zebra-striping with logarithmic age coloring. */
--blame-c1: lightgray;
--blame-c2: darkgray;
}

/**
* normalize - Remove the gray background color from active links in IE 10.
*/
Expand Down Expand Up @@ -154,10 +181,22 @@ td#line-numbers {
flex-grow: 1;
}
.source-line-with-number.stuck {
background: #ddf;
background: var(--sticky-header-bgcolor);
}
/* As documented on the variables, we use linear gradients to create an apparent
bottom border line for the last stuck element. We do this instead of using
`border-bottom: 1px solid blue` because adding a border impacts layout and
would cause a reflow. Whereas the background does not and a cross-fade can
be animated if the immediate background-change is too abrupt.
Note that we also create gradient variants for highlighted lines (because
the highlighted line can end up "stuck") and for blame display colors. */
.source-line-with-number.last-stuck {
background: linear-gradient(#ddf, #ddf 90%, blue);
background: linear-gradient(var(--sticky-header-bgcolor), var(--sticky-header-bgcolor) var(--sticky-header-bottom-border-off1), var(--sticky-header-bottom-border-color) var(--sticky-header-bottom-border-off2), var(--sticky-header-bottom-border-color));
}
.last-stuck .highlighted,
.last-stuck .multihighlight {
background: linear-gradient(var(--highlighted-bgcolor), var(--highlighted-bgcolor) var(--sticky-header-bottom-border-off1), var(--sticky-header-bottom-border-color) var(--sticky-header-bottom-border-off2), var(--sticky-header-bottom-border-color)) !important;
}

.source-line a {
Expand Down Expand Up @@ -394,6 +433,9 @@ body {
}

.content {
padding: 1rem 2rem;
}
.content.source-listing {
padding: 0.5rem 0rem;
}

Expand Down Expand Up @@ -539,20 +581,34 @@ span[data-symbols].hovered {
position: absolute;
}

/* Blame */
/* ## Blame ## */
/* the role=cell that holds the role=button `.blame-strip`. We created a class
for this so we could make it `position: relative` so it could create a new
containing block for the `.blame-popup` which cannot live beneath the
role=button `.blame-strip`. */
.blame-container {
position: relative;
}
.blame-strip {
display: block;
position: relative;
width: 20px;
height: 16px;
padding: 0;
margin: 0;
}
/* blame zebra stripes: we alternate colors whenever the revision a line is from
changes between lines. */
.c1 {
background: lightgray;
background: var(--blame-c1);
}
.c2 {
background: darkgray;
background: var(--blame-c2);
}
.last-stuck div .c1 {
background: linear-gradient(var(--blame-c1), var(--blame-c1) var(--sticky-header-bottom-border-off1), var(--sticky-header-bottom-border-color) var(--sticky-header-bottom-border-off2), var(--sticky-header-bottom-border-color));
}
.last-stuck div .c2 {
background: linear-gradient(var(--blame-c2), var(--blame-c2) var(--sticky-header-bottom-border-off1), var(--sticky-header-bottom-border-color) var(--sticky-header-bottom-border-off2), var(--sticky-header-bottom-border-color));
}
.blame-popup {
display: inline !important;
Expand Down
3 changes: 2 additions & 1 deletion static/js/blame.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,14 @@ function updateBlamePopup() {
content += `<br><details><summary>${ignored.length} ignored changesets</summary>${ignored.join("")}</details>`;
}

var parent = blameElt.parentNode;
var height = blameElt.getBoundingClientRect().height;

popupBox = document.createElement("div");
popupBox.id = "blame-popup";
popupBox.innerHTML = content;
popupBox.className = "blame-popup";
blameElt.appendChild(popupBox);
parent.appendChild(popupBox);

$(popupBox).on("mouseenter", blameHoverHandler);
$(popupBox).on("mouseleave", blameHoverHandler);
Expand Down
17 changes: 12 additions & 5 deletions static/js/code-highlighter.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@ let previouslyStuckElements = [];
* lack of discontinuity as proof that things aren't stuck. However, we can
* leverage math by making sure that a line beyond our maximum nesting level's
* line number lines up.
*
*
*/
$("#scrolling").on('scroll', function() {
const scrolling = document.getElementById('scrolling');
Expand All @@ -54,7 +52,14 @@ $("#scrolling").on('scroll', function() {
return null;
}

return parseInt(elem.dataset.lineNumber, 10);
let num = parseInt(elem.dataset.lineNumber, 10);
// In diffs, we end up with a line number of " " which parses to a NaN, and
// it's also the case in diffs that we can end up with negative line nums.
if (isNaN(num) || num < 0) {
return null;
}

return num;
}

/**
Expand Down Expand Up @@ -120,11 +125,13 @@ $("#scrolling").on('scroll', function() {
let newlyStuckElements = walkLinesAndGetLines();

let noLongerStuck = new Set(previouslyStuckElements);
let lastElem = null;
for (let elem of newlyStuckElements) {
elem.classList.add('stuck');
noLongerStuck.delete(elem);
lastElem = elem;
}
let lastElem = null;
if (newlyStuckElements.length) {
lastElem = newlyStuckElements[newlyStuckElements.length - 1];
}
let prevLastElem = null;
if (previouslyStuckElements.length) {
Expand Down
9 changes: 6 additions & 3 deletions tools/src/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,7 @@ pub fn format_file_data(
tree_name: tree_name,
include_date: env::var("MOZSEARCH_DIFFABLE").is_err(),
revision: revision,
extra_content_classes: "source-listing not-diff",
};

try!(output::generate_header(&opt, writer));
Expand Down Expand Up @@ -498,7 +499,7 @@ pub fn format_file_data(
data
} else {
// If we have no blame data, we want the div here taking up space, but we don't want it
// to both screen readers.
// to bother screen readers.
" class=\"blame-strip\" role=\"aria-hidden\"".to_owned()
};

Expand All @@ -519,7 +520,7 @@ pub fn format_file_data(
)),
F::Indent(vec![
// Blame info.
F::T(format!("<div role=\"cell\"><div{}></div></div>", blame_data)),
F::T(format!("<div role=\"cell\" class=\"blame-container\"><div{}></div></div>", blame_data)),
// The line number.
F::T(format!(
"<div id=\"l{}\" role=\"cell\" class=\"line-number\" data-line-number=\"{}\"></div>",
Expand Down Expand Up @@ -850,6 +851,7 @@ pub fn format_diff(
tree_name: tree_name,
include_date: true,
revision: Some((rev, &header)),
extra_content_classes: "source-listing diff",
};

try!(output::generate_header(&opt, writer));
Expand Down Expand Up @@ -950,7 +952,7 @@ pub fn format_diff(
F::S("<div role=\"row\" class=\"source-line-with-number\">"),
F::Indent(vec![
// Blame info.
F::T(format!("<div role=\"cell\"><div{}></div></div>", blame_data)),
F::T(format!("<div role=\"cell\" class=\"blame-container\"><div{}></div></div>", blame_data)),
// The line number and blame info.
F::T(line_str),
// The source line.
Expand Down Expand Up @@ -1129,6 +1131,7 @@ pub fn format_commit(
tree_name: tree_name,
include_date: true,
revision: None,
extra_content_classes: "commit",
};

try!(output::generate_header(&opt, writer));
Expand Down
7 changes: 6 additions & 1 deletion tools/src/output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ pub struct Options<'a> {
pub tree_name: &'a str,
pub revision: Option<(&'a str, &'a str)>,
pub include_date: bool,
/// Extra classes to include on the content element. This allows less padding to be used on
/// source listings where we have particular styling needs for "position: sticky" but want
/// every other display to have normal padding.
pub extra_content_classes: &'a str,
}

pub fn choose_icon(path: &str) -> String {
Expand Down Expand Up @@ -203,7 +207,8 @@ pub fn generate_header(opt: &Options, writer: &mut Write) -> Result<(), &'static
F::S("</div>"),

F::S(r#"<div id="scrolling">"#),
F::S(r#"<div id="content" class="content" data-no-results="No results for current query.">"#),
F::T(format!(r#"<div id="content" class="content {}" data-no-results="No results for current query.">"#,
opt.extra_content_classes)),
]),
]);

Expand Down

0 comments on commit 466d7e0

Please sign in to comment.