Skip to content

Conversation

@amrocha
Copy link
Contributor

@amrocha amrocha commented May 7, 2020

WHY are these changes introduced?

The plus global nav team wants the ability to customize the empty state of the resource list.

WHAT is this pull request doing?

Add a prop that takes in a react node to be rendered instead of the default empty state

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 May 7, 2020

🟡 This pull request modifies 5 files and might impact 6 other files. This is an average splash zone for a change, remember to tophat areas that could be affected.

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

Files potentially affected (total: 0)

📄 src/components/ResourceList/README.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/ResourceList/ResourceList.tsx (total: 0)

Files potentially affected (total: 0)

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

Files potentially affected (total: 0)

🎨 src/components/Select/Select.scss (total: 6)

Files potentially affected (total: 6)

@BPScott
Copy link
Member

BPScott commented May 7, 2020

We're on the cusp of getting #2843 reviewed and merged which converts ResourceList to a functional component. Can we get that in and rebase this PR atop of that?

@amrocha
Copy link
Contributor Author

amrocha commented May 7, 2020

@BPScott I want to ship this functionality by Monday next week, does that timeline make sense to you? If it does then I'll gladly do it

@BPScott
Copy link
Member

BPScott commented May 7, 2020

@BPScott I want to ship this functionality by Monday next week, does that timeline make sense to you? If it does then I'll gladly do it

seems possible. Liase with @dleroux and @AndrewMusgrave to see what else needs to be done in that conversion PR, perhaps merge it as-is and do a of your own pr to follow up on their remaining feedback.

I'm kinda wary of the how it looks to work with somebody on a refactor for months of them being very responsive, and us being busy and slowish to respond, and then telling them whoops sorry you need to handle this new functionality that we added in the mean time. It's gonna burn trust with folks who send us PRs

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Code looks good. Docs currently feel a bit like placeholders, with a very sparse description and not-realistic "I am an empty state" content. Would be good to get those fleshed out more.

@amrocha amrocha force-pushed the resource-list-empty-state-customization branch from ad61f60 to 02f53eb Compare May 11, 2020 19:24
Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

We can come back to improve the example content of the empty result latter :shipit:

@amrocha amrocha merged commit 2b7ac68 into master May 11, 2020
@amrocha amrocha deleted the resource-list-empty-state-customization branch May 11, 2020 21:42
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.

3 participants