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

Make entire component for push/pull tile in status bar clickable. #1534

Merged
merged 5 commits into from Jun 19, 2018

Conversation

Projects
4 participants
@annthurium
Copy link
Contributor

annthurium commented Jun 18, 2018

fixes #1530 and #1531

I don't know how to unit test this. If we had Selenium tests, or something that actually fired up the UI, that might be possible.

Test plan:

  • test clicking on the non-text area of the status bar in dev mode
  • test clicking on neighboring tiles to make sure they fire the expected actions.

@annthurium annthurium requested a review from simurai Jun 18, 2018

@@ -2,6 +2,11 @@

// Used in the status-bar

// click target should take up the width of the container
.push-pull-target {

This comment has been minimized.

@simurai

simurai Jun 19, 2018

Member

This works for the height, but for the One UI themes, the width doesn't go all the way to the edge because the parent github-PushPull element has some padding:

image

Hmm.. would it be possible to move the click target to that parent github-PushPull element? And remove the <a class="push-pull-target">?

If not or if there is a lot of refactoring needed, we could also do this:

  1. Merge this PR as is. Fixes height.
  2. Add some "negative margin hack" to the One UI themes.

Like:

.push-pull-target {
  display: block;
  margin: 0 -0.75em;
  padding: 0 0.75em;
}

This comment has been minimized.

@annthurium

annthurium Jun 19, 2018

Author Contributor

thanks for the feedback! I can take a stab at removing the pushPull element.

<Tooltip
key="tooltip"
manager={this.props.tooltipManager}
target={this.refTileNode}
title={`<div style="text-align: left; line-height: 1.2em;">${tileState.tooltip}</div>`}
showDelay={200}
hideDelay={100}
showDelay={atom.tooltips.hoverDefaults.delay.show}

This comment has been minimized.

@annthurium

annthurium Jun 19, 2018

Author Contributor

it makes me nervous using dot notation to access deeply nested object properties, without existence checks. However I think this is an atom api that's used in a lot of places, so maybe it's safe to assume this is ok.

This comment has been minimized.

@Arcanemagus

Arcanemagus Jun 22, 2018

It's not really an API, but accessing the defaults declared for the TooltipManager:

https://github.com/atom/atom/blob/8106e95150b22981f6ce55b847a0ae572014057f/src/tooltip-manager.js#L58-L60

There would have to be large changes to that class for this to change 😉.

This comment has been minimized.

@annthurium

annthurium Jun 25, 2018

Author Contributor

right right, I meant api as in interface/contract, that atom.tooltips.whatever would continue to be available. Not api as in REST api. 😄

@annthurium annthurium merged commit 61c95c2 into master Jun 19, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@annthurium annthurium deleted the tt-18-jun-tap-target branch Jun 19, 2018

@rogerhub

This comment has been minimized.

Copy link

rogerhub commented on lib/views/push-pull-view.js in 9c279ae Jul 6, 2018

If tileState is undefined, then this raises an exception. The old code handled this gracefully (because of the tileState && on line 204).

This comment has been minimized.

Copy link
Contributor Author

annthurium replied Jul 6, 2018

would you mind filing an issue with reproduction steps?

This comment has been minimized.

Copy link

rogerhub replied Jul 6, 2018

This doesn't actually affect me. I just noticed it while poking around this code. But thanks for taking a look!

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.