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

Suggested changes styling #1989

Merged
merged 5 commits into from Feb 28, 2019

Conversation

Projects
2 participants
@simurai
Copy link
Member

simurai commented Feb 27, 2019

Description of the Change

This styles the suggested changes in review comments.

Screenshot/Gif

Before After
before after

Alternate Designs

N/A

Benefits

Looks less broken. Easier to see what the suggestion is.

Possible Drawbacks

The diffs use the same colors for dark and light themes again. This could make certain themes look less optimized.

Applicable Issues

Closes #1970

Metrics

N/A

Tests

  1. Open a review comment that has a "suggested change".
  2. Verify the suggested change looks styled.

Documentation

N/A

Release Notes

N/A

User Experience Research (Optional)

N/A

@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 27, 2019

Codecov Report

Merging #1989 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1989      +/-   ##
==========================================
+ Coverage   92.12%   92.13%   +<.01%     
==========================================
  Files         188      188              
  Lines       10806    10807       +1     
  Branches     1581     1581              
==========================================
+ Hits         9955     9957       +2     
+ Misses        851      850       -1
Impacted Files Coverage Δ
lib/views/emoji-reactions-view.js 100% <0%> (ø) ⬆️
lib/containers/remote-container.js 100% <0%> (ø) ⬆️
lib/containers/issueish-detail-container.js 100% <0%> (ø) ⬆️
lib/containers/current-pull-request-container.js 95.12% <0%> (+0.12%) ⬆️
lib/containers/issueish-search-container.js 96% <0%> (+0.16%) ⬆️
lib/models/operation-states.js 91.66% <0%> (+2.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f80090...f4954a7. Read the comment docs.

@@ -58,4 +58,86 @@
vertical-align: middle;
}


// Suggested changes
.js-suggested-changes-blob {

This comment has been minimized.

@simurai

simurai Feb 27, 2019

Author Member

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.

This comment has been minimized.

@annthurium

annthurium Feb 27, 2019

Contributor

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.

simurai added some commits Feb 27, 2019

@simurai simurai requested a review from atom/github-package Feb 27, 2019

@simurai simurai marked this pull request as ready for review Feb 27, 2019

@simurai

This comment has been minimized.

Copy link
Member Author

simurai commented Feb 27, 2019

I tried to add user-select: text; and the native-key-bindings class so that the suggestion can be selected and copied, but for some reason, it copies the line above the diff (notice the jump in scroll position) and not the selection? 🤔

copy

Maybe not super necessary once it would be possible to apply the suggested change, but currently it's probably annoying having to retype the line.

// 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%)

This comment has been minimized.

@annthurium

annthurium Feb 27, 2019

Contributor

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

This comment has been minimized.

@simurai

simurai Feb 28, 2019

Author Member

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.

@@ -58,4 +58,86 @@
vertical-align: middle;
}


// Suggested changes
.js-suggested-changes-blob {

This comment has been minimized.

@annthurium

annthurium Feb 27, 2019

Contributor

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.

@simurai

This comment has been minimized.

Copy link
Member Author

simurai commented Feb 28, 2019

Ok, let's merge this as is.

@simurai simurai merged commit d42b8e8 into master Feb 28, 2019

2 checks passed

codecov/patch Coverage not affected when comparing 8f80090...f4954a7
Details
codecov/project 92.13% (+<.01%) compared to 8f80090
Details

Sprint : 13 February 2019 - 5 March 2019 : v0.27.0 automation moved this from In progress to Merged Feb 28, 2019

@simurai simurai deleted the sm/suggested-changes-styling branch Feb 28, 2019

@vanessayuenn vanessayuenn referenced this pull request Mar 6, 2019

Closed

v0.27.0-0 QA Review #2005

7 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.