Skip to content

Conversation

dleroux
Copy link
Contributor

@dleroux dleroux commented Feb 14, 2020

WHY are these changes introduced?

Fixes #2727

WHAT is this pull request doing?

  1. Ensures the right position of the Actions is correct.
  2. Changes focus state on MouseOut

resourceList

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@github-actions
Copy link
Contributor

github-actions bot commented Feb 14, 2020

🟢 This pull request modifies 4 files and might impact 2 other files.

Details:
All files potentially affected (total: 2)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🎨 src/components/ResourceItem/ResourceItem.scss (total: 2)

Files potentially affected (total: 2)

🧩 src/components/ResourceItem/ResourceItem.tsx (total: 1)

Files potentially affected (total: 1)

🧩 src/components/ResourceItem/tests/ResourceItem.test.tsx (total: 0)

Files potentially affected (total: 0)

@dleroux dleroux force-pushed the resource-item-hover-fix branch from 089614a to 58187e6 Compare February 14, 2020 16:33
@dleroux dleroux changed the title [WIP] [ResourceList] Fix persistent Action on MouseOut [ResourceList] Fix persistent Action on MouseOut Feb 14, 2020
@dleroux dleroux requested a review from loic-d February 14, 2020 16:36
Copy link
Member

@AndrewMusgrave AndrewMusgrave left a comment

Choose a reason for hiding this comment

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

LGTM will come back later today for a tophat

@dleroux dleroux force-pushed the resource-item-hover-fix branch from 58187e6 to 63a38c5 Compare February 14, 2020 16:45
@AndrewMusgrave
Copy link
Member

cc/ @chloerice

);
const wrapperDiv = resourceItem.find('div');
const className = 'ResourceItem selectable selectMode';
const focusClassName =
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit but you could add each class name into an array and .join(' ') for better readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually didn't need these in const.

Copy link
Contributor

@loic-d loic-d left a comment

Choose a reason for hiding this comment

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

Code and 🎩 👍

Noticed two little weird behaviours that I can repro on master:

Screen Shot 2020-02-14 at 12 49 22 PM

Screen Shot 2020-02-14 at 12 50 35 PM

@dleroux dleroux force-pushed the resource-item-hover-fix branch from 63a38c5 to d2fd78c Compare February 14, 2020 18:00
Copy link
Contributor Author

@dleroux dleroux left a comment

Choose a reason for hiding this comment

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

The first is expected. In a regular resource list clicking the element would navigate the user to that resource.

The second I can't reproduce. Maybe create a separate issue with steps since it's already on master.

);
const wrapperDiv = resourceItem.find('div');
const className = 'ResourceItem selectable selectMode';
const focusClassName =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually didn't need these in const.

@dleroux dleroux force-pushed the resource-item-hover-fix branch from d2fd78c to 0ca1c4f Compare February 19, 2020 13:10
@dleroux dleroux merged commit 5681bd6 into master Feb 19, 2020
@dleroux dleroux deleted the resource-item-hover-fix branch February 19, 2020 13:41
@tmlayton tmlayton temporarily deployed to production March 7, 2020 05:24 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.

[ResourceItem] shortcutActions on hover with select placing on incorrect element

4 participants