Skip to content

Conversation

attila-berki
Copy link

@attila-berki attila-berki commented Feb 14, 2020

WHY are these changes introduced?

Alignment of the <ResourceItem> content is slightly inconsistent. While the main content is center aligned all the other components like the checkbox, media and shortcut actions are set to flex-start.

I would suggest to center align all of the items, or maybe in the future enable the consumers to choose the alignment.

An issue have been reported on the product details page:

image

Sandbox example:
https://codesandbox.io/s/patient-platform-43qls?fontsize=14&hidenavigation=1&theme=dark

WHAT is this pull request doing?

This PR would enable the consumers to change the vertical item alignment of the main container, keeping the default alignment with the current flex-start.

Before:
image

After adding the alignment="center" property
image

Before:
image

After adding the alignment="center" property
image

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx and try the alignment with all of the 5 options 'leading' | 'trailing' | 'center' | 'fill' | 'baseline' :
import React from 'react';
import {
  Page,
  ResourceItem,
  ResourceList,
  Card,
  Avatar,
  TextStyle,
} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      <Card>
        <ResourceList
          resourceName={{singular: 'customer', plural: 'customers'}}
          items={[
            {
              id: 145,
              url: 'customers/145',
              avatarSource:
                'https://burst.shopifycdn.com/photos/freelance-designer-working-on-laptop.jpg?width=746',
              name: 'Yi So-Yeon',
              location: 'Gwangju, South Korea',
            },
          ]}
          selectable
          renderItem={(item) => {
            const {id, url, avatarSource, name, location} = item;
            const shortcutActions = [{content: 'View latest order', url: ''}];

            return (
              <ResourceItem
                id={id}
                url={url}
                media={
                  <Avatar
                    customer
                    size="medium"
                    name={name}
                    source={avatarSource}
                  />
                }
                accessibilityLabel={`View details for ${name}`}
                name={name}
                shortcutActions={shortcutActions}
                persistActions
                alignment="center"
              >
                <h3>
                  <TextStyle variation="strong">{name}</TextStyle>
                </h3>
                <div>{location}</div>
                <div>{id}</div>
                <div>{url}</div>
              </ResourceItem>
            );
          }}
        />
      </Card>
    </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

@ghost
Copy link

ghost commented Feb 14, 2020

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 14, 2020

🟢 This pull request modifies 5 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/README.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)

@attila-berki attila-berki changed the title [ResourceItem] Fix alignment of checkbox and media [ResourceItem] Fix vertical alignment of checkbox and media Feb 17, 2020
@chloerice
Copy link
Member

Hey @BerkiAttila 👋 Tagged in @sarahill to get some design perspective on this. From an API/code perspective I agree it'd be best to put this behind a prop for the consumer to control, since changing the alignment may be an unexpected breaking visual change.

@attila-berki
Copy link
Author

Hey @chloerice . Thank you for you feedback, I will update the PR and add a prop to control the alignment.
Although I am not sure what should be the default value. As currently it is inconsistent I would pick either center or flex-start. Maybe going with flex-start would have the least visual impact as it only affects one of the items. What do you think?

@chloerice
Copy link
Member

Hey @chloerice . Thank you for you feedback, I will update the PR and add a prop to control the alignment.
Although I am not sure what should be the default value. As currently it is inconsistent I would pick either center or flex-start. Maybe going with flex-start would have the least visual impact as it only affects one of the items. What do you think?

I'd default to the current layout of flex-start 👍(The idea behind the media and checkbox being top aligned was in order to keep things aligned across the item regardless of other custom content below the main parts of the item.)

@attila-berki
Copy link
Author

Hey @chloerice . I added the alignment as a public property with a default value of flex-start.
Would it possible to add the new property and maybe some of the existing ones to the Polaris home page? I feel it's a bit hard for the consumer to discover all of the possibilities.

Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

Hey @BerkiAttila, code and 🎩looks great! Just have a couple content suggestions, including adding an example for the new prop.

@attila-berki attila-berki force-pushed the resource-item-alignment branch from f9b92d8 to 6c83398 Compare March 2, 2020 21:48
@attila-berki
Copy link
Author

Hey @BerkiAttila, code and 🎩looks great! Just have a couple content suggestions, including adding an example for the new prop.

Hey @chloerice. Thank you for your suggestions, I have incorporated all of them. Please take another look and let me know if anything else needs to be changed.

@attila-berki attila-berki requested a review from chloerice March 3, 2020 14:31
Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

Everything 👀🏅Go ahead and merge this once you rebase @BerkiAttila 😁🚀

@attila-berki attila-berki force-pushed the resource-item-alignment branch 3 times, most recently from 1d6528c to bc09eab Compare March 4, 2020 13:36
Adding changelog entry to UNRELEASED.md

Adding alignment to the public interface of the component

Incorporating content suggestions

Update CHANGELOG.md

Revert CHANGELOG.md update

Remove already released entries from UNRELEASED.md
@attila-berki attila-berki force-pushed the resource-item-alignment branch from bc09eab to d6a0271 Compare March 4, 2020 13:37
@attila-berki attila-berki merged commit 55bae29 into master Mar 4, 2020
@attila-berki attila-berki deleted the resource-item-alignment branch March 4, 2020 13:56
@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.

3 participants