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

Table favorites #5213

Merged
merged 6 commits into from
Dec 7, 2020
Merged

Table favorites #5213

merged 6 commits into from
Dec 7, 2020

Conversation

jschuler
Copy link
Collaborator

@jschuler jschuler commented Dec 4, 2020

@patternfly-build
Copy link
Contributor

patternfly-build commented Dec 4, 2020

@mcarrano
Copy link
Member

mcarrano commented Dec 4, 2020

This looks great @jschuler . There's just one small styling issue that I wonder if we can fix. When the favorites star in the header is not selected to sort, it should be a slightly darker gray per the design. Here's what the original design looked like:

Screen Shot 2020-12-04 at 9 38 04 AM

and here's the implementation:

Screen Shot 2020-12-04 at 9 39 08 AM

Right now it feels like the column is disabled. @mcoker do we need a CSS update for that? @mceledonia can you tell us what gray value we should be using there? I don't see it specified in the design. Thanks.

@mcoker
Copy link
Contributor

mcoker commented Dec 4, 2020

@mcarrano I want to say that was something we changed while implementing the feature in core, but I can make it darker.

@mceledonia
Copy link

Yeah it should be our secondary text/global color #6A6E73 in order for it to pass contrast requirements.

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

@jschuler the styling looks right now. In looking at this again, there's just one thing I was hoping you can change. When I first click the star in the header to sort by favorities, the initial sort is to put the favorited items at the end. I need to click again to move them to the top. That's exactly opposite of what I expect. If a user sorts by favorities, it's most likely because they want to pop all of the favorited items to the top of the table so they can take some action on them. Is is possible to just turn that around?

// using this prop enables the favorites column
onFavorite={this.onFavorite}
// if the onSort prop is detected, favorites can be sorted
// if you want to exclude favorites from sorting you can use this prop with a value of `false`
Copy link
Contributor

Choose a reason for hiding this comment

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

These comments are great. I wonder if it makes sense to add them to the prop docs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, I added a description for the Favorites example that explains how to enable favoriting, and in the props I added

/**
   * Enables favorites column
   * Callback triggered when a row is favorited/unfavorited
   */
  onFavorite?: OnFavorite;
  /** Along with the onSort prop, enables favorites sorting, defaults to true */
  canSortFavorites?: boolean;

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

Looks good. Can we add some cypress interaction test as well.

/** The row index */
rowIndex?: number;
/** Additional props forwarded to the FavoritesCell */
props?: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

DO you think we should make the name more specific here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To use it you need to wrap it in the favorites object which gives it context, e.g.

favorites: {
  props: { 'aria-label': 'favs' }
}

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for making that change @jschuler .

Copy link
Contributor

@kmcfaul kmcfaul left a comment

Choose a reason for hiding this comment

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

lgtm code wise!

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

LGTM! 🤩

@jschuler jschuler merged commit a977b55 into patternfly:master Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support "favorites" in a table
7 participants