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

Do not suppress opacity transition for tooltipped-no-delay #307

Merged
merged 1 commit into from
Sep 15, 2017

Conversation

astorije
Copy link
Contributor

@astorije astorije commented Aug 5, 2017

As far as I understand (the git history is not too easy to follow on this repo :/), when tooltipped-no-delay was added here, there were no animations/transitions on the tooltip.

This commit removes the delay, so that the transition starts immediately, instead of suppressing the transition entirely. It effectively makes the 2 variants differ by only the initial delay (like the name suggests).

I noticed that on a project where both variants are used at the same time: there are tooltips on buttons (which warrant the delay), and tooltips on help icons, which have no delay. The difference in delay did not shock the eye, but the lack of transition felt off :)

@astorije
Copy link
Contributor Author

astorije commented Aug 8, 2017

I'm not entirely sure how Travis failed here. Help?

@broccolini broccolini changed the base branch from master to dev August 12, 2017 15:42
@broccolini broccolini self-assigned this Aug 12, 2017
@broccolini
Copy link
Member

Thanks @astorije I agree with this change. Sorry about the failing tests, we recently setup a new publishing flow and tests fail from forks at the moment, but we'll get that fixed soon. Also heads up for the future pr's, they should be from the dev branch rather than master. Here's the updated contribution guidelines.

I'd like to make a few other small updates to tooltips so I'll pull your changes into a new pr and will drop a link here when that's ready.

As far as I understand (the git history is not too easy to follow on this repo :/), when `tooltipped-no-delay` was added, there were no animations/transitions on the tooltip.

This commit removes the delay, so that the transition starts immediately, instead of suppressing the transition entirely. It effectively makes the 2 variants differ by only the initial delay (like the name suggests).
@astorije
Copy link
Contributor Author

Hey @broccolini, thanks for your answer!

I have rebased this branch with latest dev if you change your mind and decide to merge this PR 😊.
Re: tests, it looks like they're passing now. Not sure if you did anything on your end or if it's transient, but I'll take it 🤷‍♂️.

Sorry for opening this PR on the wrong branch. I did notice this on the CONTRIBUTING file, then it slipped my mind... Rebasing on it or starting something new locally aren't the easiest thing, because (on my local fork, at least), git checkout dev says error: pathspec 'dev' did not match any file(s) known to git.. Bizarre. May I suggest to make dev the default branch in the settings? At least people would not open PRs on the wrong branch by accident, maybe.

@broccolini
Copy link
Member

May I suggest to make dev the default branch in the settings? At least people would not open PRs on the wrong branch by accident, maybe.

We've debated this idea, we don't really like the idea of not showing Master because then if people don't notice the branch is set to dev they might confused about seeing things that are not yet published. To make it clearer that you need to branch off dev, we've added a pull request template which makes it a lot more obvious. Hopefully that helps.

I'm going to merge this into our v10 branch which we're hoping to ship next week 🎉. Thanks for contributing!

Copy link
Member

@broccolini broccolini left a comment

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants