feat: refactor Tile removing ProductTile component #548

hertweckhr1 · 2019-05-25T00:45:51Z

Description

Refactored Tile to reduce redundancy:

Product Tile now consumer Tile
ProductTileMedia now consumes TileMedia
ProductTileContent now consumes TileContent

New prop productTile(boolean) was created and passed down.

Questions to consider:

They are now refactored so product tile and its sub components are consuming Tile and its subcomponents. Is this what we want to or are we trying to eliminate productTile altogether and just have the productTile prop?

fixes #440

netlify · 2019-05-25T00:48:02Z

Deploy preview for fundamental-react ready!

Built with commit da92e76

https://deploy-preview-548--fundamental-react.netlify.com

jbadan

We should figure out a solution for the documentation of props for the ProductTile -see screenshot.

This is looking good though!

src/Tile/_ProductTileContent.js

greg-a-smith · 2019-05-29T17:39:56Z

src/Tile/Tile.js

+Tile.defaultProps = {
+    productTile: false
+};
+


If a prop's default is false, you don't need to include it in the defaultProps object -- it will automatically be evaluated as falsy.

greg-a-smith · 2019-05-29T17:41:38Z

src/Tile/_TileContent.js

    return (
        <div {...rest} className={tileContentClasses}>
-            <HeadingTag {...titleProps} className='fd-tile__title'>{title}</HeadingTag>
+            <HeadingTag {...titleProps} className={productTile ? 'fd-product-tile__title' : 'fd-tile__title'}>{title}</HeadingTag>


These should probably be extracted out into a variable that uses classnames.

greg-a-smith · 2019-05-29T18:06:38Z

src/Tile/_ProductTileContent.js

@@ -1,37 +1,10 @@
-import classnames from 'classnames';


I think this sub-component can be removed altogether as the only thing it does is pass a productTile prop to TileContent. This can be done by cloning children with the Tile component and passing the top-level productTile prop.

greg-a-smith · 2019-05-29T18:07:25Z

src/Tile/ProductTile.js

-    disabled: PropTypes.bool
-};
-
 ProductTile.Content = ProductTileContent;


This can be changed to ProductTile.Content = TileContent; (see other related comments)

greg-a-smith · 2019-05-29T18:10:53Z

src/Tile/Tile.js

    colorAccent: PropTypes.number,
    columnSpan: CustomPropTypes.range(1, 6),
    disabled: PropTypes.bool,
+    productTile: PropTypes.bool,


I think this component should clone children and pass the top-level productTile prop to each child so the prop only needs to be added at the top-level component. (see related comments)

greg-a-smith · 2019-05-29T18:12:18Z

src/Tile/_ProductTileMedia.js

 ProductTileMedia.propTypes = {
-    image: PropTypes.string.isRequired,
-    className: PropTypes.string
+    image: PropTypes.string.isRequired


In the propDescriptions for this prop, it should probably say "URL of the background image" so it's clearer to the consumer what is to be expected.

greg-a-smith · 2019-05-29T18:54:45Z

@hertweckhr1 @jbadan Ok, I've been looking at this some more. It's sort of a strange assembly of components and sub-components. Looking at the docs site examples (and playground), there seems to be three types: simple, media and product. They are very similar, but the media aspect of media and product tiles is different -- one accepts children, which seem best suited for an Image component while the other accepts a URL for a background image. Because of this, they don't follow a very good pattern for component structure (or at least not reusable).

What about something like this?

Simple:

<Tile>
    <Tile.Content title="Text">
        Some content
    </Tile.Content>
</Tile>

Media:

<Tile>
    <Tile.Media>
        <Image />
    </Tile.Media>
    <Tile.Content title="Text">
        Some content
    </Tile.Content>
</Tile>

Product:

<Tile backgroundImage="http://some.url/image" productTile>
    <Tile.Content title="Text">
        Some content
    </Tile.Content>
</Tile>

That would get rid of all the ProductTile components and sub-components (and subsequently the need to find a way to document them) and just support the product tile classes with the productTile prop on Tile. The _TileMedia component would then only be used for the media tiles.

Thoughts?

hertweckhr1 · 2019-05-30T15:20:05Z

@greg-a-smith @jbadan - this sounds good to me. Would it end up being 4 different types with Tile actions being added on as well? Or is having actions on a tile an option for all 3 simple, media, and product?

greg-a-smith · 2019-05-30T15:50:28Z

@hertweckhr1 To me, I think this really converts the different "types" of tiles to patterns rather than components. In this single component model (with sub-components), you could technically put actions or media (image) on a product tile or put a background image on a non-product tile, but those are not supported patterns. I think the examples on the docs site should just show how to use the components to create each option:

Simple tile
Simple tile with an image (media)
Simple tile with actions
Simple tile with an image (media) and actions
Product tile

greg-a-smith · 2019-06-03T15:43:43Z

src/Tile/_TileMedia.js

 TileMedia.displayName = 'Tile.Media';

 TileMedia.propTypes = {
+    backgroundImage: PropTypes.string,


I was thinking this prop would/could be on the Tile component since that already has a prop called backgroundColor. To me, TileMedia is more designed as a wrapper for children (images) rather than a URL string prop.

@greg-a-smith would you have the style tag for backgroundImage go on Tile itself then instead of media or would you pass down the backroundImage prop to media? When I originally put it on Tile, it did not format correctly: the image took up the entire background of the Title portion as well and was not big enough, so thought to keep it in Media where there is the 'fd-product-tile__media' class has previously lived maybe making it easier for the consumer to associate media with a photo in general? (whether it be background of small icon).

Either if fine with me - can you confirm if you want it on tile I should put the style tag on tile and add the fd-product-tile__media class name as well?

I think the final emitted markup would be the same as you have it now, but Tile would have the prop and would render the child <div> where it needs it to go. You are right, the way the styles are written, placing the style attribute on the top-level tile <div> would not be correct.

jbadan

This is looking great!!! I just have a few small changes.

src/Tile/Tile.Component.js

src/Tile/Tile.test.js

src/Tile/_TileMedia.js

src/Tile/__snapshots__/Tile.test.js.snap

src/_playground/documentation/Playground/Playground.js

greg-a-smith · 2019-06-03T15:52:34Z

src/Tile/Tile.js

            className={tileClasses}>
-            {children}
+            {React.Children.toArray(children).map(child => {
+                if (child.type !== TileActions) {


I think it's slightly safer to do this equality (or non-equality) check using the displayName (see TreeView code).

hertweckhr1 · 2019-06-03T18:08:07Z

src/Tile/Tile.js

-            {children}
+            {productTile &&
+                <div className='fd-product-tile__media' style={{ backgroundImage: 'url(' + backgroundImage + ')' }} />
+            }


I created a conditional for product tile rather than if the background image exists bc product tiles should have background images with them always. This will eliminate the issue Jenna mentioned with background image rendering undefined for tiles that are not product.

Well, technically it won't. There are really two props needed for a product tile: productTile and backgroundImage so it's possible the consumer passes one, but not the other. That said, this has become more of a pattern-based component so I think this is a good change. We could make productTile a shape prop with a required backgroundImage, but that might be overkill here. As we've said in this PR, the consumer just wants to know which props to pass and the example page will help them with that.

jbadan

This looks great to me!!! 🚢

One last thing, we should probably make this a feat to follow zero'th versioning breaking changes rules and add something to the title about removing ProductTile component so that it gets called out in the release notes.

A gif to represent this code review:

greg-a-smith

The code changes look great! I am just wondering about cleaning up the examples a bit. We show a disabled product tile, but the disabled prop actually works for all types of tiles. Should we add example of those? Or, (thinking out loud here) should we remove the disabled examples until the clickability of tiles is addressed. Otherwise, there is really nothing to "disable". If we did leave disabled examples, how about a single "Disabled" example with a simple, a media, a simple with actions and product tile?

We also definitely need a follow-up story to address the clickability of tiles. Adding the role="button" is adding some hover styling that would indicate the tile is clickable, however, there is nothing in the API that would handle that. It seems like an onClick prop should be passed, which then can add roles, aria labels, use different elements, etc.

Addendum: we also have to be careful about tiles with actions as a clickable element cannot also have children that are clickable.

greg-a-smith

All aboard! 🚢

hannah.van.seeters added 5 commits May 24, 2019 13:44

product tile and content complete

62182fc

media tile complete

bf5eff9

updated snapshots

9e0bf80

added title prop to product tile content

503f72f

added tests to check for className

3221d62

hertweckhr1 requested a review from a team May 28, 2019 15:47

added title prop to productTileContent tests

204e24f

jbadan requested changes May 29, 2019

View reviewed changes

src/Tile/_ProductTileContent.js Outdated Show resolved Hide resolved

hannah.van.seeters added 2 commits May 29, 2019 09:17

remove title specific prop from product tile content

029390a

edited image for product tile on docs site

d6ffac6

greg-a-smith suggested changes May 29, 2019

View reviewed changes

removed default props for productTile boolean

346ed81

hannah.van.seeters added 12 commits May 30, 2019 20:13

product tile component examples only consume tile

ba69c20

passed productTile prop by cloning children

310253c

added description for background image on media tile

040ab92

added conditional to clone to account for tile actions

4de6ab0

updated tile tests and deleted productTile.test

68fa459

product tile media tests complete with old file deleted

4045b43

deleted product tile content tests

0a3e225

product tile snapshot deleted

91af473

deleted eslint disable

2818cd8

deleted unneeded files and references to product tile

80c1dfd

media with actions example added and tileContent heading class updated

c79604f

Merge branch 'master' into 6475refactorTile

eead6c0

greg-a-smith reviewed Jun 3, 2019

View reviewed changes

jbadan requested changes Jun 3, 2019

View reviewed changes

greg-a-smith reviewed Jun 3, 2019

View reviewed changes

hannah.van.seeters added 4 commits June 3, 2019 09:28

updated example page with photo and no breaks

a5199f4

updated tests with dummy url and took away isButton

391b979

background image on tile

eae649b

deleted tile playground and fixed tests to background image change

1a33e6f

hertweckhr1 commented Jun 3, 2019

View reviewed changes

jbadan approved these changes Jun 3, 2019

View reviewed changes

hertweckhr1 requested a review from greg-a-smith June 4, 2019 15:50

greg-a-smith reviewed Jun 4, 2019

View reviewed changes

took out disbled example and role=button

da92e76

hertweckhr1 changed the title ~~fix: refactor tile component~~ feat: refactor Tile removing ProductTile component Jun 4, 2019

hertweckhr1 requested a review from greg-a-smith June 4, 2019 17:07

greg-a-smith approved these changes Jun 4, 2019

View reviewed changes

hertweckhr1 merged commit 21613bd into master Jun 4, 2019

hertweckhr1 deleted the 6475refactorTile branch June 27, 2019 18:04

feat: refactor Tile removing ProductTile component #548

feat: refactor Tile removing ProductTile component #548

Uh oh!

Conversation

hertweckhr1 commented May 25, 2019

Description

Uh oh!

netlify bot commented May 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbadan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg-a-smith commented May 29, 2019

Uh oh!

hertweckhr1 commented May 30, 2019

Uh oh!

greg-a-smith commented May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbadan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbadan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg-a-smith left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg-a-smith left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netlify bot commented May 25, 2019 •

edited

Loading

greg-a-smith commented May 30, 2019 •

edited

Loading

jbadan left a comment •

edited

Loading

greg-a-smith left a comment •

edited

Loading