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

Added layout prop to EuiCard #885

Merged
merged 7 commits into from
May 29, 2018
Merged

Added layout prop to EuiCard #885

merged 7 commits into from
May 29, 2018

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented May 29, 2018

Primarily for Kibana homepage layout

The options are "vertical" (default) or "horizontal". The caveat is that images, footers and textAlign properties are not compatible with this layout and will be ignored.

Example of a horizontal layout with badge:

screen shot 2018-05-29 at 13 52 30 pm

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Small stuff. We might want to think about a custom PropType that checks if a certain prop exists (layout), and throws an error if an incompatible one is used (textAlign). Don't think that's necessary for this PR, but might want to make an issue for it. Would be nice to have.

EuiFlexItem,
} from '../../../../src/components';

const icons = ['Beats', 'Cloud', 'Xpack'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make one of these null to show that example? I think it should probably collapse if not used?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does collapse, the problem is you're still passing <EuiIcon type="null">.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I'll change one of the examples

title={`Elastic ${item}`}
description="Example of a card's description. Stick to one or two sentences."
onClick={() => window.alert('Card clicked')}
textAlign="right"
Copy link
Contributor

Choose a reason for hiding this comment

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

Since most people will copy / paste the content, I think we probably should remove the "dead" props from the example. The callout you have is good enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, that was actually for testing, to make sure it didn't affect it, just forgot to remove it.

@snide
Copy link
Contributor

snide commented May 29, 2018

cc @nreese. This will be an even better solution for your use case.

@cchaos
Copy link
Contributor Author

cchaos commented May 29, 2018

I thought about the custom prop types too. I'll see if I can create one based on another component's custom prop type.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Please also add a render test for the horizontal layout.

@@ -12,7 +12,15 @@ const textAlignToClassNameMap = {
right: 'euiCard--rightAligned',
};

export const ALIGNMENTS = Object.keys(textAlignToClassNameMap);
export const TEXT_ALIGNMENTS = Object.keys(textAlignToClassNameMap);
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing this export name makes this a breaking change, update changelog to reflect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll just change it back, don't really need the breaking change

@cchaos
Copy link
Contributor Author

cchaos commented May 29, 2018

Ok, example without icon is created, custom prop type is created, and tests updated

@snide
Copy link
Contributor

snide commented May 29, 2018

@cchaos Awesome. 🚢

@cchaos cchaos merged commit b1513a3 into elastic:master May 29, 2018
@cchaos cchaos deleted the card-layouts branch May 29, 2018 21:45
snide pushed a commit to snide/eui that referenced this pull request May 29, 2018
snide pushed a commit to snide/eui that referenced this pull request Jun 1, 2018
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