Skip to content

Conversation

kyledurand
Copy link
Member

@kyledurand kyledurand commented Nov 28, 2018

WHY are these changes introduced?

Resolves an issue outlined in a project I'm working now where users cannot command / control click to open up list items in new tabs.

This is useful for opening more than one item at a time.

WHAT is this pull request doing?

Fixes the above ☝️

🎩 checklist

@kyledurand kyledurand requested review from a team and dleroux November 28, 2018 19:45
@ghost
Copy link

ghost commented Nov 28, 2018

👋 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 had a problem deploying to polaris-react-pr-690 November 28, 2018 19:45 Failure
@lemonmade
Copy link
Member

This isn't specifically relevant to this implementation, but why are we not just relying on native link behaviours here?

@danrosenthal
Copy link

This isn't specifically relevant to this implementation, but why are we not just relying on native link behaviours here?

Resource list items often contain links and buttons, so we have not implemented the item as a link as links can't contain other links.

@dfmcphee
Copy link
Contributor

Resource list items often contain links and buttons, so we have not implemented the item as a link as links can't contain other links.

Didn't we used to have a version that overlayed a visually hidden a tag so that you got this behaviour without the nesting?

@danrosenthal
Copy link

Didn't we used to have a version that overlayed a visually hidden a tag so that you got this behaviour without the nesting?

We do something like this in the polaris rails implementation of resource list, but that's not without its problems (issues like highlighting and selecting text.)

@BPScott
Copy link
Member

BPScott commented Nov 28, 2018

Eeeey it's that old trick.

Leveraging native link behaviour is preferable to me, as this doesn't cover middle-click to open a new tab.

@kyledurand kyledurand force-pushed the resourcelist_add-cmd-ctrl-click-support branch from ef6a998 to 347731b Compare November 28, 2018 20:59
@BPScott BPScott temporarily deployed to polaris-react-pr-690 November 28, 2018 21:00 Inactive
}

if (ctrlKey || metaKey) {
return window.open(url, '_blank');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should do a check for url because it's not required if there's an onClick.

Also I wonder, if there is an onClick and a URL should the this command click happen first and the onClick be ignored?

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured we should err on the side of caution here and allow the onClick to fire regardless.

@dleroux
Copy link
Contributor

dleroux commented Nov 30, 2018

Didn't we used to have a version that overlayed a visually hidden a tag so that you got this behaviour without the nesting?

We render a link if a URL is provided, put it behind the item content and call anchor.click(); after everything we care about in the clicking has occurred.

Copy link
Contributor

@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.

Need to update the case where's theres no url and update the unreleased.md. 🎩 looks good, thanks!

@kyledurand kyledurand force-pushed the resourcelist_add-cmd-ctrl-click-support branch from 347731b to eb6576c Compare November 30, 2018 19:57
@BPScott BPScott temporarily deployed to polaris-react-pr-690 November 30, 2018 19:57 Inactive
@kyledurand
Copy link
Member Author

Thanks for the tophat and comments @dleroux. I've updated this PR

Copy link
Contributor

@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.

👍

@kyledurand kyledurand force-pushed the resourcelist_add-cmd-ctrl-click-support branch from 13e9f58 to 53ebee1 Compare December 11, 2018 17:55
@BPScott BPScott requested a deployment to polaris-react-pr-690 December 11, 2018 17:55 Abandoned
@kyledurand kyledurand force-pushed the resourcelist_add-cmd-ctrl-click-support branch from 53ebee1 to 3302eb9 Compare December 11, 2018 18:02
@BPScott BPScott requested a deployment to polaris-react-pr-690 December 11, 2018 18:02 Abandoned
@kyledurand kyledurand merged commit 46504c1 into master Dec 11, 2018
@ghost
Copy link

ghost commented Dec 11, 2018

🎉 Thanks for your contribution to Polaris React!

@kyledurand kyledurand deleted the resourcelist_add-cmd-ctrl-click-support branch December 11, 2018 18:06
@danrosenthal danrosenthal temporarily deployed to production December 12, 2018 18:50 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