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.

Sign up for GitHub

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

Add responsiveColumn option to type of EuiDescriptionList #2166

Merged
merged 5 commits into from
Jul 26, 2019
Merged

Add responsiveColumn option to type of EuiDescriptionList #2166

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
35ae045
Add `responsive` prop to EuiDescriptionList
Jul 25, 2019
90ce6cd
Added test
Jul 25, 2019
6e0de71
Changed to use `type=responsiveColumn` instead of separate `responsiv…
Jul 26, 2019
7c5bec5
Merge branch 'master' into responsive-description-list
cchaos Jul 26, 2019
ccf46bb
Merge branch 'master' into responsive-description-list
cchaos Jul 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src-docs/src/views/description_list/description_list_column.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export default () => (
<EuiDescriptionList
type="column"
listItems={favoriteVideoGames}
responsive={true}
style={{ maxWidth: '400px' }}
/>
);
17 changes: 12 additions & 5 deletions src-docs/src/views/description_list/description_list_example.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react';
import React, { Fragment } from 'react';

import { renderToHtml } from '../../services';

Expand Down Expand Up @@ -108,10 +108,17 @@ export const DescriptionListExample = {
},
],
text: (
<p>
Using a prop <EuiCode>type</EuiCode> set to <EuiCode>column</EuiCode>{' '}
description lists can be presented in an inline, column format.
</p>
<Fragment>
<p>
Using a prop <EuiCode>type</EuiCode> set to{' '}
<EuiCode>column</EuiCode> description lists can be presented in an
inline, column format.
</p>
<p>
To return to they typical row format on smaller screens set{' '}
<EuiCode>responsive = true</EuiCode>.
</p>
</Fragment>
),
demo: <DescriptionListColumn />,
},
Expand Down
6 changes: 6 additions & 0 deletions src/components/description_list/__snapshots__/description_list.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ exports[`EuiDescriptionList props listItems is rendered as strings and elements
</dl>
`;

exports[`EuiDescriptionList props responsive is rendered 1`] = `
<dl
class="euiDescriptionList euiDescriptionList--row euiDescriptionList--responsive"
/>
`;

exports[`EuiDescriptionList props type column is rendered 1`] = `
<dl
class="euiDescriptionList euiDescriptionList--column"
Expand Down
39 changes: 34 additions & 5 deletions src/components/description_list/_description_list.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,6 @@
text-align: center;
}

&.euiDescriptionList--right {
text-align: right;
}

// Reversed makes the description larger than the title
&.euiDescriptionList--reverse {
.euiDescriptionList__title {
Expand Down Expand Up @@ -115,7 +111,6 @@
}

&.euiDescriptionList--compressed {

.euiDescriptionList__title {
@include euiTitle('xxs');
line-height: $euiLineHeight;
Expand All @@ -137,6 +132,40 @@
}
}
}

&.euiDescriptionList--responsive {
@include euiBreakpoint('xs', 's') {
display: block;

.euiDescriptionList__title,
.euiDescriptionList__description {
width: 100%;
padding: 0;
}

.euiDescriptionList__description {
@include euiFontSizeS;
margin-top: 0;
}

&.euiDescriptionList--center {
.euiDescriptionList__title,
.euiDescriptionList__description {
text-align: center;
}
}

&.euiDescriptionList--reverse {
.euiDescriptionList__title {
@include euiFontSizeS;
}

.euiDescriptionList__description {
@include euiTitle('xs');
}
}
}
}
}

&.euiDescriptionList--inline {
Expand Down
8 changes: 8 additions & 0 deletions src/components/description_list/description_list.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@ describe('EuiDescriptionList', () => {
});
});

describe('responsive', () => {
test('is rendered', () => {
const component = render(<EuiDescriptionList responsive />);

expect(component).toMatchSnapshot();
});
});

describe('type', () => {
TYPES.forEach(type => {
test(`${type} is rendered`, () => {
Expand Down
6 changes: 6 additions & 0 deletions src/components/description_list/description_list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ export interface EuiDescriptionListProps {
* Smaller text and condensed spacing
*/
compressed?: boolean;
/**
* Turns a column layout into normal row layout on small screens
*/
responsive?: boolean;
Copy link
Contributor

@snide snide Jul 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good point. How about we make a new type rather than adding the boolean then?type="responsiveColumn"

Sorry. I just really like to avoid conditional acting props because it's hard as a consumer to know how they actually change things without digging deep into the docs. Right now responsive really only changes this component if it happens to be type="column". It does nothing on row or inline. So to me, this is just a different type, not an additive property. Downside of course is if you end up adding a new type down the line where you always want to have a similar split nature, you're then adding something again, but we haven't really touched this one much over the years.

If responsive={true} by default i'd maybe think different, but if you have to go and hand touch these to turn it on anyway?

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 was more that I was trying to align the prop name/type with how we use it elsewhere. I agree that it does nothing to other display types and can create those dead selectors as you say. I don't really feel strongly either way, but I can change it to your suggestion and see how the component continues to develop (if at all). It could also a be a good test case for this way of adding responsive options.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can make the call because I can see your side of it too. If you do go with the responsive prop I'd at least make the check to only apply that class when both values match. Then at least the dom output is a little cleaner.

/**
* How should the content be styled, by default
* this will emphasize the title
Expand Down Expand Up @@ -73,6 +77,7 @@ export const EuiDescriptionList: FunctionComponent<
compressed = false,
descriptionProps,
listItems,
responsive = false,
textStyle = 'normal',
titleProps,
type = 'row',
Expand All @@ -85,6 +90,7 @@ export const EuiDescriptionList: FunctionComponent<
textStyle ? textStylesToClassNameMap[textStyle] : undefined,
{
'euiDescriptionList--compressed': compressed,
'euiDescriptionList--responsive': responsive,
},
className
);
Expand Down