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

Support nodes in EuiBasicTable column headers #1234

Merged
merged 6 commits into from
Oct 10, 2018
Merged

Support nodes in EuiBasicTable column headers #1234

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9f46aa7
Remove unnecessary header props from tables, and delete unused compre…
cjcenizal Oct 4, 2018
ba692cf
Add ability to specify nodes as EuiBasicTable headers.
cjcenizal Oct 4, 2018
826410a
Changelog
cjcenizal Oct 4, 2018
355e8ef
Caroline and her good ideas.
cjcenizal Oct 5, 2018
3dfbe72
Add back header props to table rows.
cjcenizal Oct 6, 2018
13103a3
Update changelog to be past tense.
cjcenizal Oct 6, 2018
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
## [`master`](https://github.com/elastic/eui/tree/master)

No public interface changes since `4.4.1`.
- `EuiBasicTable` accepts nodes as column headers, for supporting things like tooltips and localized text ([#1234](https://github.com/elastic/eui/pull/1234))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you re-word to be past tense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean, could you give me an example?

Copy link
Contributor

Choose a reason for hiding this comment

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

Adds support for nodes as column headers in EuiBasicTable for upporting things like tooltips and localized text.

Copy link
Contributor

Choose a reason for hiding this comment

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

ha nope!

Copy link
Contributor

Choose a reason for hiding this comment

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

Added support for nodes as column headers in EuiBasicTable for upporting things like tooltips and localized text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha thanks, will update.


## [`4.4.1`](https://github.com/elastic/eui/tree/v4.4.1)

Expand Down
6 changes: 3 additions & 3 deletions src-docs/src/views/tables/basic/props_info.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ export const propsInfo = {
name: {
description: 'The display name of the column',
required: true,
type: { name: 'string' }
type: { name: 'PropTypes.node' }
},
description: {
description: 'A description of the column (will be presented as a title over the column header',
Expand Down Expand Up @@ -213,7 +213,7 @@ export const propsInfo = {
name: {
description: 'The display name of the column',
required: false,
type: { name: 'string' }
type: { name: 'PropTypes.node' }
},
description: {
description: 'A description of the column (will be presented as a title over the column header',
Expand Down Expand Up @@ -248,7 +248,7 @@ export const propsInfo = {
name: {
description: 'The display name of the column',
required: false,
type: { name: 'string' }
type: { name: 'PropTypes.node' }
},
description: {
description: 'A description of the column (will be presented as a title over the column header',
Expand Down
217 changes: 0 additions & 217 deletions src-docs/src/views/tables/custom/compressed.js
View file
Open in desktop

This file was deleted.

4 changes: 0 additions & 4 deletions src-docs/src/views/tables/custom/custom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,6 @@ export default class extends Component {
return (
<EuiTableRowCell
key={column.id}
header={column.label}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why you removed all these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I should have explained this in the PR description. I came across this and noticed that it's not the correct attribute name (it should be headers). But after some more digging it looks like this is not a recommend practice and isn't even necessary for accessibility. VoiceOver reads the header value for each cell just fine. So after chatting with @lukeelmers we decided to just remove them.

Copy link
Contributor

Choose a reason for hiding this comment

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

EuiTableRowCell receives the header prop for displaying the table header inline for mobile. It could probably use a better name (this was one of my early foray's into React). But it is necessary.

eui/src/components/table/table_row_cell.js

Lines 87 to 90 in 0cf8fde

/**
* The column's header title for use in mobile view (will be added as a data-attr)
*/
header: PropTypes.string,

Copy link
Contributor

@cchaos cchaos Oct 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

EuiTableRowCell receives the header prop for displaying the table header inline for mobile.

Ahhhh that makes much more sense then! @cjcenizal and I were going back and forth on this and assumed it was intended to be an actual headers attribute and was actually a typo.

It could probably use a better name

+1 -- I think that could help prevent confusion in the future

Copy link
Contributor Author

@cjcenizal cjcenizal Oct 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yup what Luke said! I'll put it back in the meantime and add it to the propTypes definition if it's not already there.

textOnly={false}
hasActions={true}
align="right"
Expand Down Expand Up @@ -527,7 +526,6 @@ export default class extends Component {
return (
<EuiTableRowCell
key={column.id}
header={column.label}
align={column.alignment}
truncateText={cell && cell.truncateText}
textOnly={cell ? cell.textOnly : true}
Expand Down Expand Up @@ -581,7 +579,6 @@ export default class extends Component {
footers.push(
<EuiTableFooterCell
key={`footer_${column.id}`}
header={column.title}
align={column.alignment}
>
{footer}
Expand All @@ -591,7 +588,6 @@ export default class extends Component {
footers.push(
<EuiTableFooterCell
key={`footer_empty_${footers.length - 1}`}
header={column.title}
align={column.alignment}
>
{undefined}
Expand Down
54 changes: 49 additions & 5 deletions src-docs/src/views/tables/sorting/sorting.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
import React, {
Component
Component,
} from 'react';
import { formatDate } from '../../../../../src/services/format';
import { createDataStore } from '../data_store';

import {
EuiBasicTable,
EuiLink,
EuiFlexGroup,
EuiFlexItem,
EuiHealth,
EuiIconTip,
EuiLink,
} from '../../../../../src/components';

/*
Expand Down Expand Up @@ -100,27 +103,68 @@ export class Table extends Component {
}, {
field: 'github',
name: 'Github',
Copy link
Member

Choose a reason for hiding this comment

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

I guess you meant to remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove what?

Copy link
Member

Choose a reason for hiding this comment

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

The name property (name: 'Github',)

name: (
<EuiFlexGroup gutterSize="xs" alignItems="center">
Copy link
Contributor

Choose a reason for hiding this comment

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

Add responsive={false} to fix in mobile

<EuiFlexItem>
Github
</EuiFlexItem>

<EuiFlexItem>
<EuiIconTip content="Their mascot is the Octokitty" />
</EuiFlexItem>
</EuiFlexGroup>
),
render: (username) => (
<EuiLink href={`https://github.com/${username}`} target="_blank">
{username}
</EuiLink>
)
}, {
field: 'dateOfBirth',
name: 'Date of Birth',
name: (
<EuiFlexGroup gutterSize="xs" alignItems="center">
<EuiFlexItem>
Date of Birth
</EuiFlexItem>

<EuiFlexItem>
<EuiIconTip content="Colloquially known as a 'birthday'" />
</EuiFlexItem>
</EuiFlexGroup>
),
dataType: 'date',
render: (date) => formatDate(date, 'dobLong'),
sortable: true
}, {
field: 'nationality',
name: 'Nationality',
name: (
<EuiFlexGroup gutterSize="xs" alignItems="center">
<EuiFlexItem>
Nationality
</EuiFlexItem>

<EuiFlexItem>
<EuiIconTip content="The nation in which this person resides" />
</EuiFlexItem>
</EuiFlexGroup>
),
render: (countryCode) => {
const country = store.getCountry(countryCode);
return `${country.flag} ${country.name}`;
}
}, {
field: 'online',
name: 'Online',
name: (
<EuiFlexGroup gutterSize="xs" alignItems="center">
<EuiFlexItem>
Online
</EuiFlexItem>

<EuiFlexItem>
<EuiIconTip content="Free to talk or busy with business" />
</EuiFlexItem>
</EuiFlexGroup>
),
dataType: 'boolean',
render: (online) => {
const color = online ? 'success' : 'danger';
Expand Down
Loading