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

BUG Add a status column to the table view #980

Merged

Conversation

maxime-rainville
Copy link
Contributor

@maxime-rainville maxime-rainville force-pushed the pulls/1/status-on-table-view branch from d00f13f to 82a4b16 Compare August 29, 2019 06:40
* @param {object} props
* @returns {Component|null}
*/
renderStatus(props) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was inspired by getStatusFlags on GalleryItem.

getStatusFlags has a updateStatusFlags hook that looks like it's meant to allow module like Fluent to add their own status flags. However, it's not clear how you would go about using it or that it even works.

If allowing module to add their own flags is a key requirements, I think we need to rethink how this is done and come up with a consistent way of doing for both the gallery view and the table view.

client/src/containers/TableView/TableView.js Outdated Show resolved Hide resolved

flags = flags.map(({ ...attributes }) => <VersionedBadge {...attributes} />);

return <span>{flags}</span>;
Copy link
Contributor

Choose a reason for hiding this comment

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

So this would return an empty span for folders - was that deliberate?

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've updated it to return null when there's no flag. There's a possible future where we update this again so other module can specify their own flags and where someone decide they want to flag folders.

@maxime-rainville maxime-rainville force-pushed the pulls/1/status-on-table-view branch from 871678d to e5f206b Compare September 2, 2019 03:22
@bergice
Copy link
Contributor

bergice commented Sep 5, 2019

LGTM, just gotta get the tests green @maxime-rainville

@maxime-rainville maxime-rainville merged commit 3510cc0 into silverstripe:1 Sep 10, 2019
@maxime-rainville maxime-rainville deleted the pulls/1/status-on-table-view branch September 10, 2019 01:12
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