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

GH-1623: Update Hover State for Expanded View #408

Merged
merged 5 commits into from Jul 17, 2019

Conversation

@tdtnguyen
Copy link
Contributor

@tdtnguyen tdtnguyen commented Jul 2, 2019

  • Have you followed the guidelines in CONTRIBUTING.md?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what your changes do?
  • Does your submission pass tests?
  • Did you lint your code prior to submission?
@tdtnguyen tdtnguyen requested a review from ghostery/ghostery as a code owner Jul 2, 2019
Copy link
Contributor

@IAmThePan IAmThePan left a comment

I left one comment but otherwise everything looks good!

top: 215px;
z-index: 3;
cursor: pointer;
background-position: center;

This comment has been minimized.

@IAmThePan

IAmThePan Jul 2, 2019
Contributor

Try using this to move the left caret to the left:

background-position: calc(50% - 1.75px);

you may have to have separate css selector for the right arrow and left.

@tdtnguyen tdtnguyen requested a review from IAmThePan Jul 3, 2019
@tdtnguyen tdtnguyen changed the base branch from master to develop Jul 3, 2019
@christophertino christophertino added this to the 8.4.1 milestone Jul 3, 2019
Copy link
Contributor

@IAmThePan IAmThePan left a comment

I think it looks good but could be slightly better by moving the arrow by another pixel. Run this past Vinny tomorrow and once you get sign off by design I will merge the Pull Request.

@@ -97,11 +97,13 @@
background-repeat: no-repeat;
background-image: url('../../app/images/panel/line.svg');
&:hover {
background-position: calc(50% - 1.75px);

This comment has been minimized.

@IAmThePan

IAmThePan Jul 8, 2019
Contributor

I think this looks better with:

background-position: calc(50% - 2px);
app/scss/partials/_detail.scss Show resolved Hide resolved
@tdtnguyen tdtnguyen requested a review from IAmThePan Jul 9, 2019
Copy link
Contributor

@IAmThePan IAmThePan left a comment

Approved. Waiting to merge until we get the OK from Tino.
We may not want to merge now because there are some tentative fixes that might need to get made before we start merging 8.4.1 features.

IAmThePan and others added 2 commits Jul 9, 2019
@christophertino christophertino merged commit ccdd55f into develop Jul 17, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@christophertino christophertino deleted the expandedViewHoverState branch Jul 17, 2019
@tdtnguyen tdtnguyen changed the title updated expanded view hover state GH-1623: Update Hover State for Expanded View Jul 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants