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

[ExceptionList] Change warning status items from orange to yellow #813

Merged
merged 2 commits into from
Jan 8, 2019

Conversation

chloerice
Copy link
Member

@chloerice chloerice commented Jan 2, 2019

WHY are these changes introduced?

Fixes #804

WHAT is this pull request doing?

Updates the font, icon, and bullet colors of warning status ExceptionList items from dark orange to dark yellow.

Before After
screen shot 2019-01-04 at 11 51 55 am screen shot 2019-01-04 at 11 50 59 am

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,
  ResourceList,
  Card,
  ExceptionList,
  TextStyle,
} from '@shopify/polaris';

interface State {
  selectedItems: any[];
}

export default class Playground extends React.Component<{}, State> {
  state: State = {
    selectedItems: [],
  };

  handleSelectionChange = (selectedItems: any[]) => {
    this.setState({selectedItems});
  };

  renderItem = (item, id) => {
    const {orderId, address} = item;
    const exceptions =
      id === 256
        ? [
            {
              status: 'critical',
              icon: 'risk',
              title: 'High fraud risk',
            },
            {
              status: 'warning',
              icon: 'alert',
              title: 'No available inventory',
              description: '-2 in stock',
            },
            {
              icon: 'notes',
              title: 'This customer is awesome',
            },
          ]
        : [];
    return (
      <ResourceList.Item
        id={id}
        accessibilityLabel={`View details for ${name}`}
      >
        <h3>
          <TextStyle variation="strong">{orderId}</TextStyle>
        </h3>
        <div>{address}</div>
        <div style={{marginLeft: '-4px', marginTop: '4px'}}>
          <ExceptionList items={exceptions} />
        </div>
      </ResourceList.Item>
    );
  };

  render() {
    const resourceName = {
      singular: 'order',
      plural: 'orders',
    };

    const items = [
      {
        id: 341,
        orderId: '120341',
        address: 'Decatur, USA',
      },
      {
        id: 256,
        orderId: '120256',
        name: 'Ellen Ochoa',
        address: 'Los Angeles, USA',
      },
    ];

    const promotedBulkActions = [
      {
        content: 'Edit orders',
        onAction: () => console.log('Todo: implement bulk edit'),
      },
    ];

    const bulkActions = [
      {
        content: 'Add tags',
        onAction: () => console.log('Todo: implement bulk add tags'),
      },
      {
        content: 'Remove tags',
        onAction: () => console.log('Todo: implement bulk remove tags'),
      },
      {
        content: 'Archive orders',
        onAction: () => console.log('Todo: implement bulk archive'),
      },
    ];

    return (
      <Page
        title="Orders"
        primaryAction={{content: 'View Examples', url: '/examples'}}
      >
        <Card>
          <ResourceList
            resourceName={resourceName}
            items={items}
            renderItem={this.renderItem}
            selectedItems={this.state.selectedItems}
            onSelectionChange={this.handleSelectionChange}
            promotedBulkActions={promotedBulkActions}
            bulkActions={bulkActions}
          />
        </Card>
      </Page>
    );
  }
}

🎩 checklist

@BPScott BPScott temporarily deployed to polaris-react-pr-813 January 2, 2019 21:34 Inactive
@chloerice chloerice force-pushed the exceptionlist-warning-darkyellow branch from bce4ac3 to fa01c78 Compare January 2, 2019 21:35
@BPScott BPScott temporarily deployed to polaris-react-pr-813 January 2, 2019 21:35 Inactive
Copy link
Contributor

@dpersing dpersing left a comment

Choose a reason for hiding this comment

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

It looks like in the original issue, the comparison was for a white background. Here, against the light gray background, the yellow technically doesn't pass (it's 4.12:1) for normal sized text (which I should have caught since that's what's used in the actual example in the style guide, doh).

Unfortunately the next darkest shade up in both the yellow and orange families (Darker) just kind of looks brown next to the red example, which doesn't really convey the orange-y warning-ness.

Dark yellow against the light gray background in dev tools

Thoughts, @sarahill?

@chloerice
Copy link
Member Author

chloerice commented Jan 3, 2019

It looks like in the original issue, the comparison was for a white background. Here, against the light gray background, the yellow technically doesn't pass (it's 4.12:1) for normal sized text (which I should have caught since that's what's used in the actual example in the style guide, doh).

Oy 😣 I've updated the playground code to be more realistic to make it easier to see the various states/conditions the exception list component can be in. It's typically used inside of a ResourceList.Item or inside of a critical Banner (listing multiple form validation errors at the top of a detail page). The hover, selected, and focus states of ResourceList.Item likely give even worse contrast results than the gray background. @dpersing is that a Chrome extension you're using to inspect the contrast ratio?

@sarahill
Copy link
Contributor

sarahill commented Jan 3, 2019

Ok, I messed around with this a bit. I tried using the darker yellows and yellow text but those get muddy and really dark. Reducing the brightness of yellow dark looks the best and passes contrast standards. Thoughts?

And thanks for catching this!

image

@dpersing
Copy link
Contributor

dpersing commented Jan 3, 2019

is that a Chrome extension you're using to inspect the contrast ratio?

@chloerice It's built into Chrome now (was in Canary for a long time)! If you click on a text color swatch in the CSS window it does its best to calculate the contrast based on what the background color appears to be.

@dpersing
Copy link
Contributor

dpersing commented Jan 3, 2019

Reducing the brightness of yellow dark looks the best and passes contrast standards.

@sarahill That works for me. Would it be possible to update the hex in the UI kit and style guide to match #8A6166?

@sarahill
Copy link
Contributor

sarahill commented Jan 3, 2019

@dpersing I was wondering if we should just update the hex value for yellow dark. It will affect icons and some text in the UI but I don't think it will be a glaringly obvious change that will impact negatively.

@chloerice can you update the value in polaris-react? I'll create issues for the UI Kit, Style guide and Polaris Tokens.

image

@chloerice
Copy link
Member Author

@chloerice It's built into Chrome now (was in Canary for a long time)! If you click on a text color swatch in the CSS window it does its best to calculate the contrast based on what the background color appears to be.

Ahhh okay! (I was looking for it in the "Accessibility" section of the dev tools.)

@chloerice
Copy link
Member Author

@chloerice can you update the value in polaris-react? I'll create issues for the UI Kit, Style guide and Polaris Tokens.

@sarahill On it!!

@chloerice
Copy link
Member Author

@sarahill Actually it looks like the only change needed to update the color across the board (polaris-react, polaris-rails, and polaris-styleguide) is the update you're working on in polaris-tokens 👊

@sarahill
Copy link
Contributor

sarahill commented Jan 3, 2019

@chloerice Ah that makes sense! I'm on it 👊

@sarahill
Copy link
Contributor

sarahill commented Jan 4, 2019

Ok, the update to color-yellow-dark has been merged in polaris-tokens Shopify/polaris-tokens#44

@chloerice chloerice force-pushed the exceptionlist-warning-darkyellow branch from fa01c78 to a56080f Compare January 4, 2019 19:37
@BPScott BPScott temporarily deployed to polaris-react-pr-813 January 4, 2019 19:37 Inactive
@chloerice
Copy link
Member Author

chloerice commented Jan 4, 2019

Ok, the update to color-yellow-dark has been merged in polaris-tokens Shopify/polaris-tokens#44

Awesome!! 👏 @dpersing @sarahill I've added an upgrade to the new release of polaris-tokens to this PR if you could please re-review 😃.

@chloerice chloerice force-pushed the exceptionlist-warning-darkyellow branch from a56080f to a646692 Compare January 4, 2019 19:49
@BPScott BPScott temporarily deployed to polaris-react-pr-813 January 4, 2019 19:49 Inactive
@dpersing
Copy link
Contributor

dpersing commented Jan 4, 2019

This looks great to me! Thank you so much, @sarahill and @chloerice!

@@ -34,6 +35,8 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f

### Dependency upgrades

- Upgraded `@shopify/polaris-tokens` to v2.1.1 ([#813](https://github.com/Shopify/polaris-react/pull/813))
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@sarahill sarahill left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@danrosenthal danrosenthal temporarily deployed to production January 8, 2019 15:33 Inactive
@kaelig kaelig deleted the exceptionlist-warning-darkyellow branch February 21, 2019 00:50
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.

5 participants