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

feat(card): create CardWithFile high-level component #2208

Merged
merged 2 commits into from
Jul 24, 2019

Conversation

bryceosterhaus
Copy link
Member

@bryceosterhaus bryceosterhaus commented Jul 23, 2019

Fixes #2195
Fixes #2196
Fixes #2197

@coveralls
Copy link

coveralls commented Jul 23, 2019

Pull Request Test Coverage Report for Build 3035

  • 34 of 35 (97.14%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 85.074%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/clay-drop-down/src/DropDownWithBasicItems.tsx 0 1 0.0%
Totals Coverage Status
Change from base Build 3028: 0.006%
Covered Lines: 1580
Relevant Lines: 1751

💛 - Coveralls

const headerContent = (
<ClayCard.AspectRatio className="card-item-first">
<div className="aspect-ratio-item aspect-ratio-item-center-middle aspect-ratio-item-fluid card-type-asset-icon">
<ClayIcon spritemap={spritemap} symbol="documents-and-media" />
Copy link
Member

Choose a reason for hiding this comment

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

We will have to make symbol configurable here, the user or the product that using can modify it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, what would you imagine users changing it to? I was looking in the docs and it seemed explicit that this was the symbol someone would see.

Copy link
Member

Choose a reason for hiding this comment

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

The icons may be different, so we as a library should let the product team set this up easily without holding them back so much that they have to issue an issue here just to change the icon I want to leave this part of the high-level not too rigid in order not to reach that level, to certain things that we can make configurable.

But in the documentation has the description of making this configurable, are you looking at this document https://docs.google.com/document/d/1st2moaRVphrsFrDJUz-ic26aH-V46YkQam75X6BHPY0/edit#heading=h.eevh9ngq3hi8?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, I wasn't able to find that doc. that should help. thanks

<ClayCard.Body>
<div className="autofit-col">
<ClaySticker inline>
<ClayIcon spritemap={spritemap} symbol="folder" />
Copy link
Member

Choose a reason for hiding this comment

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

I would say the same here

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'd be hesitant to make it configurable since its explicitly the "CardWithFolder" component. If we remove that symbol, it starts to be much more vague.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, you're right about that, you might want to rename it CardWithHorizontal to follow what Lexicon said: https://docs.google.com/document/d/1st2moaRVphrsFrDJUz-ic26aH-V46YkQam75X6BHPY0/edit#heading=h.x89wughhxazs

What do you think?

const interactiveTag = interactive ? 'span' : 'div';

const TagName = href ? 'a' : interactiveTag;
const OuterTag = CARD_TYPE_ELEMENTS[displayType] as ('span' | 'div' | 'h3');
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, why isn't TypeScript inferring the output here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will infer the type of the value, string, but it wouldn't pick up the explicit string values.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah, this is true.

Copy link
Contributor

@wincent wincent Aug 2, 2019

Choose a reason for hiding this comment

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

@bryceosterhaus, @matuzalemsteles.

There are two ways to force it to infer a better type here.

Best way is to use as const like this:

const CARD_TYPE_ELEMENTS = {
	subtitle: 'span',
	text: 'div',
	title: 'h3',
} as const;

This stops TypeScript from widening the type to string and you can drop the cast.

The other way that I can think of is to explicitly annotate the type, but that is very repetitive and I wouldn't suggest it:

type T = {
	subtitle: 'span',
	text: 'div',
	title: 'h3',
};

const CARD_TYPE_ELEMENTS: T = {
	subtitle: 'span',
	text: 'div',
	title: 'h3',
};

Playground link

Copy link
Member

Choose a reason for hiding this comment

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

Best way is to use as const like this...

This is a good approach. Thanks @wincent

Copy link
Contributor

Choose a reason for hiding this comment

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

I sent a PR with the suggested fix.

@bryceosterhaus
Copy link
Member Author

@matuzalemsteles made some changes. I also implemented the ability to use images for files, since file and image cards are essentially the same. I also went through that doc and updated the cards to be slightly more flexible for config. Let me know what you think

)}

{imgProps && (
<img
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure CardWithFile will accept images in case it should only accept icons, but I understand that in the end it looks like CardWithImage maybe we can leave a more generic name to merge the two into one.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, yeah probably smart to make the name more generic.

ClayCardWithBasicInfo
or
ClayCardWithEntity (kinda weird cause we already have *WithUser)

We could try to merge this with ClayCardWithUser and make it ClayCardWithEntity. Not sure if we want to keep the distinction though.

Any other names come to mind?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm I don't know if I like ClayCardWithEntity does not seem so obvious to me its use, maybe going with ClayCardWithInfo it is not quite basic so in fact we can remove the word Basic.

@bryceosterhaus
Copy link
Member Author

@matuzalemsteles i went with CardWithInfo. Simple enough and flexible enough for future improvements

Copy link
Member

@matuzalemsteles matuzalemsteles left a comment

Choose a reason for hiding this comment

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

LGTM! I think we can change the ClayCard Migration Guide to use these components now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants