Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Suggested changes styling #1989

Merged
simurai merged 5 commits into from
Feb 28, 2019
Merged

Suggested changes styling #1989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e5fd7e8
Add right padding to comments
simurai Feb 26, 2019
5406bf1
Style suggested changes
simurai Feb 27, 2019
81a2d2c
:memo: typo
simurai Feb 27, 2019
521fb20
Style info tooltip
simurai Feb 27, 2019
f4954a7
Fix margins
simurai Feb 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 7 additions & 23 deletions styles/file-patch-view.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,32 +183,16 @@
// Line decorations

&-line {
// mixin
.hunk-line-mixin(@color) {
background-color: fade(@color, 15%);

&--deleted {
background-color: @github-diff-deleted;
&.line.cursor-line {
background-color: fade(@color, 25%);
}
}

// light themes
& when (lightness(@syntax-background-color) >= 50) {
&--deleted {
.hunk-line-mixin( hsl(353, 100%, 55%) ); // close to .com's hsl(353, 100%, 93%)
}
&--added {
.hunk-line-mixin( hsl(133, 100%, 50%) ); // close to .com's hsl(133, 100%, 90%)
background-color: fadein(@github-diff-deleted, 3%);
}
}

// dark themes
& when (lightness(@syntax-background-color) < 50) {
&--deleted {
.hunk-line-mixin( hsl(353, 100%, 65%) ); // close to .com's hsl(353, 100%, 93%)
}
&--added {
.hunk-line-mixin( hsl(133, 40%, 60%) ); // close to .com's hsl(133, 100%, 90%)
&--added {
background-color: @github-diff-added;
&.line.cursor-line {
background-color: fadein(@github-diff-added, 3%);
}
}
}
Expand Down
6 changes: 5 additions & 1 deletion styles/github-dotcom-markdown.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

// This styles Markdown used in issueish panes.

@margin: 1.5em;
@margin: 1em;

.github-DotComMarkdownHtml {

Expand Down Expand Up @@ -179,4 +179,8 @@
color: @text-color-subtle; // same as .cross-referenced-event-label-number
}

.js-suggested-changes-blob { // gets used for suggested changes
margin: @margin 0;
}

}
149 changes: 148 additions & 1 deletion styles/pr-comment.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@


.github-PrCommentThread {
padding: @component-padding 0;
padding: @component-padding @component-padding @component-padding 0;
}


Expand Down Expand Up @@ -58,4 +58,151 @@
vertical-align: middle;
}


// Suggested changes
.js-suggested-changes-blob {
Copy link
Contributor Author

@simurai simurai Feb 27, 2019 •
edited by annthurium
Loading

Choose a reason for hiding this comment

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

I assume we don't have much control over the markup that is coming from the GitHub API?

screen shot 2019-02-27 at 3 28 47 pm

.js-suggested-changes-blob shouldn't be used for styling, but it seems like the best identifier to scope "suggested changes". Also, the class names from .com could change anytime and might break the styling in Atom.

I'm ok with shipping this as is, but it might be good to add a TODO or file an issue to communicate that this solution is somewhat fragile.

Copy link

@annthurium annthurium Feb 27, 2019

Choose a reason for hiding this comment

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

yeah, I thought about how styling based on this classname is fragile 😦

On the plus side, no functionality would be broken...it would just look janky again.

The "right" way to do this is to identify "suggested changes" comments based on data attributes that come from the graphQL api (which is probably possible, but I haven't looked at the schema.) . Then we apply our own classname.

font-size: .9em;
border: 1px solid @base-border-color;
border-radius: @component-border-radius;

.d-flex { display: flex; }
.d-inline-block { display: inline-block; }
.flex-auto { flex: 1 1 auto; }
.p-1 { padding: @component-padding/2; }
.px-1 { padding-left: @component-padding/2; padding-right: @component-padding/2; }
.p-2 { padding: @component-padding; }
.text-gray { color: @text-color-subtle; }
.lh-condensed { line-height: 1.25; }
.rounded-1 { border-radius: @component-border-radius; }
.border { border: 1px solid; }
.border-bottom { border-bottom: 1px solid @base-border-color; }
.border-green { border-color: @text-color-success; }
.octicon { vertical-align: -4px; fill: currentColor; }

.blob-wrapper > table {
width: 100%;
margin: 0;
font-family: var(--editor-font-family);
font-size: var(--editor-font-size);
line-height: var(--editor-line-height);

td {
border: none;
padding: 0 .5em;
}
}

.blob-num {
padding: 0 .5em;
color: @text-color-subtle;
&:before {
content: attr(data-line-number);
}
&-addition {
background-color: fadein(@github-diff-added, 10%);
}
&-deletion {
background-color: fadein(@github-diff-deleted, 10%);
}
}

.blob-code {
&-inner {
&:before { margin-right: .5em; }
.x {
&-first {
border-bottom-left-radius: @component-border-radius;
border-top-left-radius: @component-border-radius;
}
&-last {
border-bottom-right-radius: @component-border-radius;
border-top-right-radius: @component-border-radius;
}
}
}

&-addition {
background-color: @github-diff-added;
&:before { content: "+"; }
.x {
background-color: @github-diff-added-highlight;
}
}

&-deletion {
background-color: @github-diff-deleted;
&:before { content: "-"; }
.x {
background-color: @github-diff-deleted-highlight;
}
}
}

// These styles are mostly copied from the Primer tooltips
.tooltipped {
position: relative;

// This is the tooltip bubble
&::after {
position: absolute;
z-index: 1000000;
display: none;
width: max-content;
max-width: 22em;
padding: .5em .6em;
font-weight: 500;
color: @base-background-color;
text-align: center;
pointer-events: none;
content: attr(aria-label);
background: @background-color-info;
border-radius: @component-border-radius;
}

// This is the tooltip arrow
&::before {
position: absolute;
z-index: 1000001;
display: none;
width: 0;
height: 0;
color: @background-color-info;
pointer-events: none;
content: "";
border: 6px solid transparent;
}

// This will indicate when we'll activate the tooltip
&:hover,
&:active,
&:focus {
&::before,
&::after {
display: inline-block;
text-decoration: none;
}
}

// Tooltipped south
&::after {
top: 100%;
right: 50%;
margin-top: 6px;
}
&::before {
top: auto;
right: 50%;
bottom: -7px;
margin-right: -6px;
border-bottom-color: @background-color-info;
}

// Move the tooltip body to the center of the object.
&::after {
transform: translateX(50%);
}
}

}

}
10 changes: 10 additions & 0 deletions styles/variables.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,13 @@
@gh-background-color-red: #dc3545;
@gh-background-color-purple: #6f42c1;
@gh-background-color-green: #28a745;


// diff colors -----------------
// Needs to be semi transparent to make it work with selections and different themes

@github-diff-deleted: fade(hsl(353, 100%, 66%), 15%); // similar to .com's hsl(353, 100%, 97%)
Copy link

@annthurium annthurium Feb 27, 2019

Choose a reason for hiding this comment

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

so cool. I really want to learn more about css colors.

Copy link
Contributor Author

@simurai simurai Feb 28, 2019

Choose a reason for hiding this comment

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

If possible, it's best to only use the UI and syntax theme variables. Then it should look ok in all themes.

But sometimes making an exception is fine too. 😬 In this case, it felt better to use the .com colors for the diff and not "success" and "error". But it needed to be semi transparent to make the selections visible underneath. Less has a bunch of functions to tweak colors, like fade() to make an existing color more transparent.

I also have been using mix() sometimes. For example

.class {
  background-color: mix(@text-color, @base-background-color, 10%);
}

mixes 10% of the text color with the background color -> making the new background color slightly darker with light themes or slightly lighter in dark themes.

@github-diff-added: fade(hsl(137, 100%, 55%), 15%); // similar to .com's hsl(137, 100%, 95%)

@github-diff-deleted-highlight: fade(hsl(353, 95%, 66%), 25%); // similar to .com's hsl(353, 95%, 86%)
@github-diff-added-highlight: fade(hsl(135, 73%, 55%), 25%); // similar to .com's hsl(135, 73%, 81%)