Skip to content

Conversation

johanstromqvist
Copy link
Contributor

@johanstromqvist johanstromqvist commented Apr 18, 2019

WHY are these changes introduced?

Tag button hover state should be immediate.

WHAT is this pull request doing?

Removes excessive hover state transition on tag button.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import * as React from 'react';
import {Page} from '@shopify/polaris';

interface State {}

export default class Playground extends React.Component<never, State> {
  render() {
    return (
      <Page title="Playground">
        {/* Add the code you want to test here */}
      </Page>
    );
  }
}

🎩 checklist

@ghost
Copy link

ghost commented Apr 18, 2019

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines. You can also join #polaris on the Shopify Partners Slack.

@BPScott BPScott temporarily deployed to polaris-react-pr-1337 April 18, 2019 20:10 Inactive
@danrosenthal
Copy link

Hey @johanstromqvist, would you mind providing a bit of rationale for this change, and pinging a couple reviewers?

cc: @ry5n

@ry5n
Copy link
Contributor

ry5n commented May 6, 2019

I’d want to understand the rationale as well.

@danrosenthal
Copy link

@johanstromqvist one more bump for context on this change

@johanstromqvist
Copy link
Contributor Author

johanstromqvist commented May 13, 2019

Sorry about the delay guys – just got back from vacation today. Expect rationale within the next unknown amount of days, or let me know if it's urgent and blocking you. I'll also provide more rationale in future PRs.

Edit: 🤷‍♂️

@johanstromqvist
Copy link
Contributor Author

Sorry about the delay.

The short story is that the touch target is behaving inconsistently and instead of fixing it I simply removed the behaviour. I asked around in the Polaris team but we couldn't find any rationale for the transition, and I couldn't see any myself. We could see tho that the component hadn't been updated in a long time.

Why do I think transition is excessive
The transition doesn't solve any problems or improve UX in my opinion, and I can see how adding transitions to hover states can add a sluggish delay which distances users from direct control.

Why do I suggest hover to be immediate
As a rule of thumb, I'd suggest that hover feedback should default to immediate, unless the hover triggers state change that would benefit from clarification. Before suggesting that as a general guideline, I'd like to try out on a couple of scenarios and run it by you guys. It's my current opinion tho.

The inconsistency
The current tag button has a 100-200ms transition on the background of the touch target and 0ms transition on the x icon in the centre, causing it to feel wonky and uncrafted.

- Updated open styleguide pr to create multiple pull requests to update `polaris-react` across multiple repos ([#1069](https://github.com/Shopify/polaris-react/pull/1069))
- Updated the pull request creation to retry when it fails ([#1069](https://github.com/Shopify/polaris-react/pull/1069))
- Exported overlay and layer data attributes for use in consumer components ([#1266](https://github.com/Shopify/polaris-react/pull/1266))
- Removed transition on tag button hover state [#1337](https://github.com/Shopify/polaris-react/pull/1337)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Removed transition on tag button hover state [#1337](https://github.com/Shopify/polaris-react/pull/1337)
- Removed transition on tag button hover state ([#1337](https://github.com/Shopify/polaris-react/pull/1337))

Copy link
Contributor

@tmlayton tmlayton left a comment

Choose a reason for hiding this comment

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

I support this change, the transition seems unnecessary.

Copy link

@danrosenthal danrosenthal left a comment

Choose a reason for hiding this comment

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

Thanks for the context @johanstromqvist!

@danrosenthal danrosenthal force-pushed the tag-button-transition branch from 3fa4f37 to 2dfc210 Compare May 31, 2019 14:32
Co-authored-by: johanstromqvist <johan.stromqvist@shopify.com>
@danrosenthal danrosenthal force-pushed the tag-button-transition branch from 2dfc210 to 3beaa27 Compare May 31, 2019 14:38
@danrosenthal danrosenthal merged commit 53a52fb into master May 31, 2019
@ghost
Copy link

ghost commented May 31, 2019

🎉 Thanks for your contribution to Polaris React!

@danrosenthal danrosenthal deleted the tag-button-transition branch May 31, 2019 14:49
@dleroux dleroux temporarily deployed to production June 5, 2019 14:15 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants